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

Change-Id: I039422a65374cf0ccbe0073441f0e8c442ebf830
BUG: 1399134
Signed-off-by: Raghavendra G <rgowdapp@redhat.com>
Reviewed-on: http://review.gluster.org/15945
Smoke: Gluster Build System <jenkins@build.gluster.org>
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
Reviewed-by: N Balachandran <nbalacha@redhat.com>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Reviewed-by: Shyamsundar Ranganathan <srangana@redhat.com>

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 719d214f92d..be9d2ce5e4e 100644
--- a/xlators/cluster/dht/src/dht-common.h
+++ b/xlators/cluster/dht/src/dht-common.h
@@ -580,6 +580,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 66e3ede736b..f8e614a40aa 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 46bf461cf63..f13762e34fb 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;