On Wed, Mar 06, 2013 at 12:19:14PM +0100, Lars Ellenberg wrote: > From: Lars Ellenberg <lars@xxxxxxxxxx> > > If for some reason glusterd_get_brick_root() fails, > it frees the gf_strdup'ed *mount_point in its own error path, > and returns -1. > > Unfortunately it already had assigned that pointer value > to the output argument, the caller function > glusterd_add_brick_detail() sees a non-NULL pointer, > and free() again: segfault. > > Could be fixed with a one-liner (*mount_point = NULL) > in the error path, but I think glusterd_get_brick_root() > should only assign to the output argument once all checks passed, > so I use a local temporary pointer, which increases the patch a bit. > > Signed-off-by: Lars Ellenberg <lars@xxxxxxxxxx> https://bugzilla.redhat.com/show_bug.cgi?id=919352 http://review.gluster.org/#change,4646 You may want to consider to add a "double free" check, you already have (at least two flavors of) instrumented free(), depending on ->mem_acct_enable... Below is what I mean. You need to flesh out the do_something_useful_log_loud_and_screaming_but_not_segfault() part, still ;-) Thanks, Lars --- libglusterfs/src/mem-pool.h 2013-03-08 10:26:55.902715232 +0100 *************** *** 97,103 **** #define FREE(ptr) \ ! if (ptr != NULL) { \ free ((void *)ptr); \ ! ptr = (void *)0xeeeeeeee; \ ! } --- 97,108 ---- + #define DANGLING_PTR_MAGIC ((void*)0xeeeeeeee) #define FREE(ptr) \ ! do { \ ! if (ptr == DANGLING_PTR_MAGIC) { \ ! do_something_useful_log_loud_and_screaming_but_not_segfault(); \ ! } else if (ptr != NULL) { \ free ((void *)ptr); \ ! ptr = DANGLING_PTR_MAGIC; \ ! } \ ! } while (0)
