From 53b24b46c6e265f0d30e46ad635d09dbddaade3b Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Tue, 7 Jun 2011 05:50:55 +0000 Subject: pump: cleanup potential dict related memory corruption. Signed-off-by: Krishnan Parthasarathi Signed-off-by: Anand Avati BUG: 2489 (GlusterFS crashing with replace-brick) URL: http://bugs.gluster.com/cgi-bin/bugzilla3/show_bug.cgi?id=2489 --- libglusterfs/src/mem-types.h | 3 ++- libglusterfs/src/syncop.c | 5 ++-- xlators/cluster/afr/src/afr-common.c | 18 ++++++++++--- xlators/cluster/afr/src/afr-self-heal-data.c | 32 +++++++++++++++++------ xlators/cluster/afr/src/afr-self-heal-entry.c | 37 ++++++++++++++++++++++----- xlators/cluster/afr/src/pump.c | 33 ++++++++++++++++-------- 6 files changed, 97 insertions(+), 31 deletions(-) diff --git a/libglusterfs/src/mem-types.h b/libglusterfs/src/mem-types.h index 9840dac43..0ebfc9ece 100644 --- a/libglusterfs/src/mem-types.h +++ b/libglusterfs/src/mem-types.h @@ -98,6 +98,7 @@ enum gf_common_mem_types_ { gf_common_mt_sge = 73, gf_common_mt_rpcclnt_cb_program_t = 74, gf_common_mt_libxl_marker_local = 75, - gf_common_mt_end = 76 + gf_common_mt_int32_t = 76, + gf_common_mt_end = 77 }; #endif diff --git a/libglusterfs/src/syncop.c b/libglusterfs/src/syncop.c index 267e4b3a2..a341b92d9 100644 --- a/libglusterfs/src/syncop.c +++ b/libglusterfs/src/syncop.c @@ -291,7 +291,7 @@ syncop_lookup_cbk (call_frame_t *frame, void *cookie, xlator_t *this, if (op_ret == 0) { args->iatt1 = *iatt; - args->xattr = xattr; + args->xattr = dict_ref (xattr); args->iatt2 = *parent; } @@ -313,10 +313,11 @@ syncop_lookup (xlator_t *subvol, loc_t *loc, dict_t *xattr_req, if (iatt) *iatt = args.iatt1; if (xattr_rsp) - *xattr_rsp = args.xattr; + *xattr_rsp = dict_ref (args.xattr); if (parent) *parent = args.iatt2; + dict_unref (args.xattr); errno = args.op_errno; return args.op_ret; } diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c index c52b04be3..6a12d744d 100644 --- a/xlators/cluster/afr/src/afr-common.c +++ b/xlators/cluster/afr/src/afr-common.c @@ -63,14 +63,26 @@ int32_t afr_set_dict_gfid (dict_t *dict, uuid_t gfid) { - int ret = 0; + int ret = 0; + uuid_t *pgfid = NULL; GF_ASSERT (gfid); - ret = dict_set_static_bin (dict, "gfid-req", gfid, 16); - if (ret) + pgfid = GF_CALLOC (1, sizeof (uuid_t), gf_common_mt_char); + if (!pgfid) { + ret = -1; + gf_log (THIS->name, GF_LOG_ERROR, "Out of memory"); + goto out; + } + uuid_copy (*pgfid, gfid); + + ret = dict_set_dynptr (dict, "gfid-req", pgfid, 16); + if (ret) { + GF_FREE (pgfid); gf_log (THIS->name, GF_LOG_DEBUG, "gfid set failed"); + } +out: return ret; } diff --git a/xlators/cluster/afr/src/afr-self-heal-data.c b/xlators/cluster/afr/src/afr-self-heal-data.c index 13aa054dc..3791f21c1 100644 --- a/xlators/cluster/afr/src/afr-self-heal-data.c +++ b/xlators/cluster/afr/src/afr-self-heal-data.c @@ -844,7 +844,7 @@ afr_sh_data_fxattrop (call_frame_t *frame, xlator_t *this) afr_local_t *local = NULL; afr_private_t *priv = NULL; dict_t *xattr_req = NULL; - int32_t zero_pending[3] = {0,}; + int32_t *zero_pending = NULL; int call_count = 0; int i = 0; int ret = 0; @@ -859,13 +859,23 @@ afr_sh_data_fxattrop (call_frame_t *frame, xlator_t *this) local->call_count = call_count; xattr_req = dict_new(); - if (xattr_req) { - for (i = 0; i < priv->child_count; i++) { - ret = dict_set_static_bin (xattr_req, priv->pending_key[i], - zero_pending, 3 * sizeof(int32_t)); - if (ret < 0) - gf_log (this->name, GF_LOG_WARNING, - "Unable to set dict value"); + if (!xattr_req) { + gf_log (this->name, GF_LOG_ERROR, "Out of memory"); + goto out; + } + + for (i = 0; i < priv->child_count; i++) { + zero_pending = GF_CALLOC (3, sizeof (int32_t), gf_common_mt_int32_t); + if (!zero_pending) { + gf_log (this->name, GF_LOG_ERROR, "Out of memory"); + goto out; + } + ret = dict_set_dynptr (xattr_req, priv->pending_key[i], + zero_pending, 3 * sizeof (int32_t)); + if (ret < 0) { + GF_FREE (zero_pending); + gf_log (this->name, GF_LOG_WARNING, + "Unable to set dict value"); } } @@ -883,8 +893,14 @@ afr_sh_data_fxattrop (call_frame_t *frame, xlator_t *this) } } + dict_unref (xattr_req); + return 0; + +out: if (xattr_req) dict_unref (xattr_req); + sh->op_failed = 1; + afr_sh_data_done (frame, this); return 0; } diff --git a/xlators/cluster/afr/src/afr-self-heal-entry.c b/xlators/cluster/afr/src/afr-self-heal-entry.c index 95356ff7c..b54b33194 100644 --- a/xlators/cluster/afr/src/afr-self-heal-entry.c +++ b/xlators/cluster/afr/src/afr-self-heal-entry.c @@ -1056,7 +1056,7 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, call_frame_t *frame = NULL; int active_src = 0; int child_index = 0; - int pending_array[3] = {0, }; + int32_t *pending_array = NULL; dict_t *xattr = NULL; int ret = 0; int idx = 0; @@ -1088,9 +1088,21 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, inode->ia_type = stbuf->ia_type; - xattr = get_new_dict (); - dict_ref (xattr); + xattr = dict_new (); + if (!xattr) { + sh->op_failed = 1; + gf_log (this->name, GF_LOG_ERROR, "Out of memory"); + goto out; + } + pending_array = (int32_t*) GF_CALLOC (3, sizeof (int32_t), + gf_common_mt_int32_t); + + if (!pending_array) { + sh->op_failed = 1; + gf_log (this->name, GF_LOG_ERROR, "Out of memory"); + goto out; + } idx = afr_index_for_transaction_type (AFR_METADATA_TRANSACTION); pending_array[idx] = hton32 (1); if (IA_ISDIR (stbuf->ia_type)) @@ -1099,11 +1111,13 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, idx = afr_index_for_transaction_type (AFR_DATA_TRANSACTION); pending_array[idx] = hton32 (1); - ret = dict_set_static_bin (xattr, priv->pending_key[child_index], - pending_array, sizeof (pending_array)); - if (ret < 0) + ret = dict_set_dynptr (xattr, priv->pending_key[child_index], + pending_array, 3 * sizeof (int32_t)); + if (ret < 0) { + GF_FREE (pending_array); gf_log (this->name, GF_LOG_WARNING, "Unable to set dict value."); + } valid = GF_SET_ATTR_ATIME | GF_SET_ATTR_MTIME; parentbuf = impunge_sh->parentbuf; @@ -1111,6 +1125,11 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, parent_loc = GF_CALLOC (1, sizeof (*parent_loc), gf_afr_mt_loc_t); + if (!parent_loc) { + sh->op_failed = 1; + gf_log (this->name, GF_LOG_ERROR, "Out of memory"); + goto out; + } afr_build_parent_loc (parent_loc, &impunge_local->loc); STACK_WIND_COOKIE (impunge_frame, afr_sh_entry_impunge_xattrop_cbk, @@ -1130,6 +1149,12 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, return 0; out: + if (xattr) + dict_unref (xattr); + + if (pending_array) + GF_FREE (pending_array); + LOCK (&impunge_frame->lock); { call_count = --impunge_local->call_count; diff --git a/xlators/cluster/afr/src/pump.c b/xlators/cluster/afr/src/pump.c index 99d57f2a5..6a932e74e 100644 --- a/xlators/cluster/afr/src/pump.c +++ b/xlators/cluster/afr/src/pump.c @@ -860,7 +860,8 @@ pump_initiate_sink_connect (call_frame_t *frame, xlator_t *this) afr_local_t *local = NULL; afr_private_t *priv = NULL; dict_t *dict = NULL; - char *dst_brick = NULL; + data_t *data = NULL; + char *clnt_cmd = NULL; loc_t loc = {0}; int ret = 0; @@ -872,8 +873,8 @@ pump_initiate_sink_connect (call_frame_t *frame, xlator_t *this) build_root_loc (priv->root_inode, &loc); - ret = dict_get_str (local->dict, PUMP_CMD_START, &dst_brick); - if (ret < 0) { + data = data_ref (dict_get (local->dict, PUMP_CMD_START)); + if (!data) { gf_log (this->name, GF_LOG_ERROR, "Could not get destination brick value"); goto out; @@ -887,12 +888,21 @@ pump_initiate_sink_connect (call_frame_t *frame, xlator_t *this) goto out; } - GF_ASSERT (dst_brick); - gf_log (this->name, GF_LOG_DEBUG, - "Got destination brick as %s", dst_brick); + clnt_cmd = GF_CALLOC (1, data->len+1, gf_common_mt_char); + if (!clnt_cmd) { + gf_log (this->name, GF_LOG_ERROR, + "Out of memory"); + goto out; + } + + memcpy (clnt_cmd, data->data, data->len); + clnt_cmd[data->len] = '\0'; + gf_log (this->name, GF_LOG_DEBUG, "Got destination brick %s\n", + clnt_cmd); - ret = dict_set_str (dict, CLIENT_CMD_CONNECT, dst_brick); + ret = dict_set_dynstr (dict, CLIENT_CMD_CONNECT, clnt_cmd); if (ret < 0) { + GF_FREE (clnt_cmd); gf_log (this->name, GF_LOG_ERROR, "Could not inititiate destination brick " "connect"); @@ -911,6 +921,8 @@ pump_initiate_sink_connect (call_frame_t *frame, xlator_t *this) dict_unref (dict); out: + if (data) + data_unref (data); return ret; } @@ -1034,10 +1046,10 @@ pump_execute_status (call_frame_t *frame, xlator_t *this) dict = dict_new (); - ret = dict_set_str (dict, PUMP_CMD_STATUS, dict_str); + ret = dict_set_dynptr (dict, PUMP_CMD_STATUS, dict_str, PATH_MAX + 256); if (ret < 0) { gf_log (this->name, GF_LOG_DEBUG, - "dict_set_str returned negative value"); + "dict_set_dynptr returned negative value"); } op_ret = 0; @@ -1047,7 +1059,6 @@ out: AFR_STACK_UNWIND (getxattr, frame, op_ret, op_errno, dict); dict_unref (dict); - GF_FREE (dict_str); return 0; } @@ -2430,7 +2441,7 @@ init (xlator_t *this) while (i < child_count) { priv->children[i] = trav->xlator; - ret = asprintf (&priv->pending_key[i], "%s.%s", AFR_XATTR_PREFIX, + ret = gf_asprintf (&priv->pending_key[i], "%s.%s", AFR_XATTR_PREFIX, trav->xlator->name); if (-1 == ret) { gf_log (this->name, GF_LOG_ERROR, -- cgit