From 998ccd5de71df70a611595166fb914c7e7fbe130 Mon Sep 17 00:00:00 2001 From: N Balachandran 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 Signed-off-by: N Balachandran Reviewed-on: https://code.engineering.redhat.com/gerrit/92555 Reviewed-by: Atin Mukherjee Tested-by: Atin Mukherjee --- 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