From 998ccd5de71df70a611595166fb914c7e7fbe130 Mon Sep 17 00:00:00 2001
From: N Balachandran <nbalacha@redhat.com>
Date: Fri, 9 Dec 2016 11:04:13 +0530
Subject: [PATCH 234/235] cluster/dht: Fix memory corruption while accessing
regex stored in private
If reconfigure is executed parallely (or concurrently with dht_init),
there are races that can corrupt memory. One such race is modification
of regexes stored in conf (conf->rsync_regex_valid and
conf->extra_regex_valid) through dht_init_regex. With change [1],
reconfigure codepath can get executed parallely (with itself or with
dht_init) and this fix is needed.
Also, a reconfigure can race with any thread doing dht_layout_search,
resulting in dht_layout_search accessing regex freed up by reconfigure
(like in bz 1399134).
[1] http://review.gluster.org/15046
Upstream patches:
master: http://review.gluster.org/#/c/15945/
release-3.8 : http://review.gluster.org/#/c/15793/
release-3.9 : http://review.gluster.org/#/c/15949/
Change-Id: I62daadc5886338b61107e20920a2727d510c4c84
BUG: 1399100
Original-author: Raghavendra G <rgowdapp@redhat.com>
Signed-off-by: N Balachandran <nbalacha@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/92555
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
Tested-by: Atin Mukherjee <amukherj@redhat.com>
---
xlators/cluster/dht/src/dht-common.h | 1 +
xlators/cluster/dht/src/dht-hashfn.c | 54 ++++++++++++++++++++++--------------
xlators/cluster/dht/src/dht-shared.c | 54 ++++++++++++++++++++----------------
3 files changed, 64 insertions(+), 45 deletions(-)
diff --git a/xlators/cluster/dht/src/dht-common.h b/xlators/cluster/dht/src/dht-common.h
index 227dc08..e87bd90 100644
--- a/xlators/cluster/dht/src/dht-common.h
+++ b/xlators/cluster/dht/src/dht-common.h
@@ -551,6 +551,7 @@ struct dht_conf {
/* lock migration */
gf_boolean_t lock_migration_enabled;
+ gf_lock_t lock;
};
typedef struct dht_conf dht_conf_t;
diff --git a/xlators/cluster/dht/src/dht-hashfn.c b/xlators/cluster/dht/src/dht-hashfn.c
index 66e3ede..f8e614a 100644
--- a/xlators/cluster/dht/src/dht-hashfn.c
+++ b/xlators/cluster/dht/src/dht-hashfn.c
@@ -41,12 +41,16 @@ dht_hash_compute_internal (int type, const char *name, uint32_t *hash_p)
static
gf_boolean_t
-dht_munge_name (const char *original, char *modified, size_t len, regex_t *re)
+dht_munge_name (const char *original, char *modified,
+ size_t len, regex_t *re)
{
- regmatch_t matches[2];
- size_t new_len;
+ regmatch_t matches[2] = {{0}, };
+ size_t new_len = 0;
+ int ret = 0;
- if (regexec(re,original,2,matches,0) != REG_NOMATCH) {
+ ret = regexec(re, original, 2, matches, 0);
+
+ if (ret != REG_NOMATCH) {
if (matches[1].rm_so != -1) {
new_len = matches[1].rm_eo - matches[1].rm_so;
/* Equal would fail due to the NUL at the end. */
@@ -60,7 +64,7 @@ dht_munge_name (const char *original, char *modified, size_t len, regex_t *re)
}
/* This is guaranteed safe because of how the dest was allocated. */
- strcpy(modified,original);
+ strcpy(modified, original);
return _gf_false;
}
@@ -68,28 +72,36 @@ int
dht_hash_compute (xlator_t *this, int type, const char *name, uint32_t *hash_p)
{
char *rsync_friendly_name = NULL;
- dht_conf_t *priv = this->private;
+ dht_conf_t *priv = NULL;
size_t len = 0;
gf_boolean_t munged = _gf_false;
- if (priv->extra_regex_valid) {
- len = strlen(name) + 1;
- rsync_friendly_name = alloca(len);
- munged = dht_munge_name (name, rsync_friendly_name, len,
- &priv->extra_regex);
- }
+ priv = this->private;
- if (!munged && priv->rsync_regex_valid) {
- len = strlen(name) + 1;
- rsync_friendly_name = alloca(len);
- gf_msg_trace (this->name, 0, "trying regex for %s", name);
- munged = dht_munge_name (name, rsync_friendly_name, len,
- &priv->rsync_regex);
- if (munged) {
- gf_msg_debug (this->name, 0,
- "munged down to %s", rsync_friendly_name);
+ LOCK (&priv->lock);
+ {
+ if (priv->extra_regex_valid) {
+ len = strlen(name) + 1;
+ rsync_friendly_name = alloca(len);
+ munged = dht_munge_name (name, rsync_friendly_name, len,
+ &priv->extra_regex);
+ }
+
+ if (!munged && priv->rsync_regex_valid) {
+ len = strlen(name) + 1;
+ rsync_friendly_name = alloca(len);
+ gf_msg_trace (this->name, 0, "trying regex for %s",
+ name);
+ munged = dht_munge_name (name, rsync_friendly_name, len,
+ &priv->rsync_regex);
+ if (munged) {
+ gf_msg_debug (this->name, 0,
+ "munged down to %s",
+ rsync_friendly_name);
+ }
}
}
+ UNLOCK (&priv->lock);
if (!munged) {
rsync_friendly_name = (char *)name;
diff --git a/xlators/cluster/dht/src/dht-shared.c b/xlators/cluster/dht/src/dht-shared.c
index 48ec9ff..40f8832 100644
--- a/xlators/cluster/dht/src/dht-shared.c
+++ b/xlators/cluster/dht/src/dht-shared.c
@@ -336,9 +336,9 @@ out:
}
void
dht_init_regex (xlator_t *this, dict_t *odict, char *name,
- regex_t *re, gf_boolean_t *re_valid)
+ regex_t *re, gf_boolean_t *re_valid, dht_conf_t *conf)
{
- char *temp_str;
+ char *temp_str = NULL;
if (dict_get_str (odict, name, &temp_str) != 0) {
if (strcmp(name,"rsync-hash-regex")) {
@@ -347,25 +347,29 @@ dht_init_regex (xlator_t *this, dict_t *odict, char *name,
temp_str = "^\\.(.+)\\.[^.]+$";
}
- if (*re_valid) {
- regfree(re);
- *re_valid = _gf_false;
- }
+ LOCK (&conf->lock);
+ {
+ if (*re_valid) {
+ regfree(re);
+ *re_valid = _gf_false;
+ }
- if (!strcmp(temp_str,"none")) {
- return;
- }
+ if (!strcmp(temp_str, "none")) {
+ goto unlock;
+ }
- if (regcomp(re,temp_str,REG_EXTENDED) == 0) {
- gf_msg_debug (this->name, 0,
- "using regex %s = %s", name, temp_str);
- *re_valid = _gf_true;
- }
- else {
- gf_msg (this->name, GF_LOG_WARNING, 0,
- DHT_MSG_REGEX_INFO,
- "compiling regex %s failed", temp_str);
+ if (regcomp(re, temp_str, REG_EXTENDED) == 0) {
+ gf_msg_debug (this->name, 0,
+ "using regex %s = %s", name, temp_str);
+ *re_valid = _gf_true;
+ } else {
+ gf_msg (this->name, GF_LOG_WARNING, 0,
+ DHT_MSG_REGEX_INFO,
+ "compiling regex %s failed", temp_str);
+ }
}
+unlock:
+ UNLOCK (&conf->lock);
}
int
@@ -486,9 +490,9 @@ dht_reconfigure (xlator_t *this, dict_t *options)
}
dht_init_regex (this, options, "rsync-hash-regex",
- &conf->rsync_regex, &conf->rsync_regex_valid);
+ &conf->rsync_regex, &conf->rsync_regex_valid, conf);
dht_init_regex (this, options, "extra-hash-regex",
- &conf->extra_regex, &conf->extra_regex_valid);
+ &conf->extra_regex, &conf->extra_regex_valid, conf);
GF_OPTION_RECONF ("weighted-rebalance", conf->do_weighting, options,
bool, out);
@@ -627,6 +631,10 @@ dht_init (xlator_t *this)
goto err;
}
+ LOCK_INIT (&conf->subvolume_lock);
+ LOCK_INIT (&conf->layout_lock);
+ LOCK_INIT (&conf->lock);
+
/* We get the commit-hash to set only for rebalance process */
if (dict_get_uint32 (this->options,
"commit-hash", &commit_hash) == 0) {
@@ -776,17 +784,15 @@ dht_init (xlator_t *this)
}
dht_init_regex (this, this->options, "rsync-hash-regex",
- &conf->rsync_regex, &conf->rsync_regex_valid);
+ &conf->rsync_regex, &conf->rsync_regex_valid, conf);
dht_init_regex (this, this->options, "extra-hash-regex",
- &conf->extra_regex, &conf->extra_regex_valid);
+ &conf->extra_regex, &conf->extra_regex_valid, conf);
ret = dht_layouts_init (this, conf);
if (ret == -1) {
goto err;
}
- LOCK_INIT (&conf->subvolume_lock);
- LOCK_INIT (&conf->layout_lock);
conf->gen = 1;
--
2.9.3