From 25da481bc5b06d671e41e5a70b2c145777154bf1 Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Fri, 10 Jun 2011 04:42:28 +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 +- xlators/cluster/afr/src/afr-common.c | 14 +++++++-- xlators/cluster/afr/src/afr-self-heal-data.c | 38 ++++++++++++++-------- xlators/cluster/afr/src/afr-self-heal-entry.c | 43 +++++++++++++++++-------- xlators/cluster/afr/src/pump.c | 45 ++++++++++++++++++++------- 5 files changed, 103 insertions(+), 40 deletions(-) diff --git a/libglusterfs/src/mem-types.h b/libglusterfs/src/mem-types.h index 3a55bdc7c..5fb1f8095 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/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c index 868dab736..5c8a8105d 100644 --- a/xlators/cluster/afr/src/afr-common.c +++ b/xlators/cluster/afr/src/afr-common.c @@ -63,13 +63,23 @@ 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); + pgfid = GF_CALLOC (1, sizeof (uuid_t), gf_common_mt_char); + if (!pgfid) { + ret = -1; + goto out; + } + uuid_copy (*pgfid, gfid); - ret = dict_set_static_bin (dict, "gfid-req", gfid, 16); + ret = dict_set_dynptr (dict, "gfid-req", pgfid, 16); if (ret) gf_log (THIS->name, GF_LOG_DEBUG, "gfid set failed"); +out: + if (ret && pgfid) + GF_FREE (pgfid); 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 5adaba1c9..d4516c4ce 100644 --- a/xlators/cluster/afr/src/afr-self-heal-data.c +++ b/xlators/cluster/afr/src/afr-self-heal-data.c @@ -859,12 +859,11 @@ afr_sh_data_fxattrop_cbk (call_frame_t *frame, void *cookie, int afr_sh_data_fxattrop (call_frame_t *frame, xlator_t *this) { - afr_self_heal_t *sh = NULL; - afr_local_t *local = NULL; - afr_private_t *priv = NULL; - dict_t *xattr_req = NULL; - - int32_t zero_pending[3] = {0, 0, 0}; + afr_self_heal_t *sh = NULL; + afr_local_t *local = NULL; + afr_private_t *priv = NULL; + dict_t *xattr_req = NULL; + int32_t *zero_pending = NULL; int call_count = 0; int i = 0; @@ -880,14 +879,21 @@ 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) { + ret = -1; + 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) { + ret = -1; + goto out; } + ret = dict_set_dynptr (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"); } for (i = 0; i < priv->child_count; i++) { @@ -904,8 +910,14 @@ afr_sh_data_fxattrop (call_frame_t *frame, xlator_t *this) } } +out: if (xattr_req) dict_unref (xattr_req); + if (ret) { + GF_FREE (zero_pending) + 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 a4077ddb0..0c9fca495 100644 --- a/xlators/cluster/afr/src/afr-self-heal-entry.c +++ b/xlators/cluster/afr/src/afr-self-heal-entry.c @@ -1040,14 +1040,14 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, struct iatt *preparent, struct iatt *postparent) { - int call_count = 0; - afr_private_t *priv = NULL; - afr_local_t *impunge_local = NULL; - afr_self_heal_t *impunge_sh = NULL; - call_frame_t *frame = NULL; - int active_src = 0; - int child_index = 0; - int pending_array[3] = {0, }; + int call_count = 0; + afr_private_t *priv = NULL; + afr_local_t *impunge_local = NULL; + afr_self_heal_t *impunge_sh = NULL; + call_frame_t *frame = NULL; + int active_src = 0; + int child_index = 0; + int32_t *pending_array = NULL; dict_t *xattr = NULL; int ret = 0; int idx = 0; @@ -1080,8 +1080,16 @@ 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) { + ret = -1; + goto out; + } + pending_array = GF_CALLOC (3, sizeof (int32_t), gf_common_mt_int32_t); + if (!pending_array) { + ret = -1; + goto out; + } idx = afr_index_for_transaction_type (AFR_METADATA_TRANSACTION); pending_array[idx] = hton32 (1); @@ -1101,8 +1109,11 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, parentbuf = impunge_sh->parentbuf; setattr_frame = copy_frame (impunge_frame); - parent_loc = GF_CALLOC (1, sizeof (*parent_loc), - gf_afr_mt_loc_t); + parent_loc = GF_CALLOC (1, sizeof (*parent_loc), gf_afr_mt_loc_t); + if (!parent_loc) { + ret = -1; + goto out; + } afr_build_parent_loc (parent_loc, &impunge_local->loc); STACK_WIND_COOKIE (impunge_frame, afr_sh_entry_impunge_xattrop_cbk, @@ -1122,6 +1133,12 @@ afr_sh_entry_impunge_newfile_cbk (call_frame_t *impunge_frame, void *cookie, return 0; out: + if (ret) { + if (xattr) + dict_unref (xattr); + if (pending_array) + GF_FREE (pending_array); + } LOCK (&impunge_frame->lock); { call_count = --impunge_local->call_count; @@ -1861,7 +1878,7 @@ afr_sh_entry_impunge_readdir_cbk (call_frame_t *frame, void *cookie, return 0; } - + int afr_sh_entry_impunge_subvol (call_frame_t *frame, xlator_t *this, diff --git a/xlators/cluster/afr/src/pump.c b/xlators/cluster/afr/src/pump.c index 10b9b7203..a7df543b4 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; int ret = 0; @@ -872,8 +873,9 @@ 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) { + ret = -1; gf_log (this->name, GF_LOG_ERROR, "Could not get destination brick value"); goto out; @@ -887,11 +889,16 @@ pump_initiate_sink_connect (call_frame_t *frame, xlator_t *this) goto out; } - GF_ASSERT (dst_brick); + clnt_cmd = GF_CALLOC (1, data->len+1, gf_common_mt_char); + if (!clnt_cmd) { + ret = -1; + goto out; + } + memcpy (clnt_cmd, data->data, data->len); gf_log (this->name, GF_LOG_DEBUG, - "Got destination brick as %s", dst_brick); + "Got destination brick as %s", 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_log (this->name, GF_LOG_ERROR, "Could not inititiate destination brick " @@ -909,8 +916,14 @@ pump_initiate_sink_connect (call_frame_t *frame, xlator_t *this) ret = 0; - dict_unref (dict); out: + if (dict) + dict_unref (dict); + if (data) + data_unref (data); + if (ret) + GF_FREE (clnt_cmd); + return ret; } @@ -1019,7 +1032,6 @@ pump_execute_status (call_frame_t *frame, xlator_t *this) if (!dict_str) { gf_log (this->name, GF_LOG_ERROR, "Out of memory"); - op_ret = -1; op_errno = ENOMEM; goto out; } @@ -1033,21 +1045,32 @@ pump_execute_status (call_frame_t *frame, xlator_t *this) } dict = dict_new (); + if (!dict) { + ret = -1; + op_errno = ENOMEM; + goto out; + } - ret = dict_set_str (dict, PUMP_CMD_STATUS, dict_str); + ret = dict_set_dynstr (dict, PUMP_CMD_STATUS, dict_str); if (ret < 0) { + op_errno = -ret; gf_log (this->name, GF_LOG_DEBUG, "dict_set_str returned negative value"); + goto out; } op_ret = 0; out: + if (ret) + op_ret = -1; AFR_STACK_UNWIND (getxattr, frame, op_ret, op_errno, dict); - dict_unref (dict); - GF_FREE (dict_str); + if (dict) + dict_unref (dict); + if (ret) + GF_FREE (dict_str); return 0; } -- cgit