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