[DRBD-user] DRBD 8.4.1: drbdadm crashes with glibc MALLOC_CHECK_

[Lars Ellenberg]

On Thu, Jul 12, 2012 at 09:26:30AM +0000, BRESTAN Rainer wrote:
> Using drbdadm with environment variable MALLOC_CHECK_ set to not zero result in segmentation fault.
> 
> Reason is the handling of most free operations in drbdadm_main.c, notably in functions free_config, free_host_info and maybe some other more.
> 
> Example: function free_host_info
>         for_each_volume(vol, hi->volumes)
>                 free_volume(vol);
> 
> Function free_volume call as last libc function free(vol), which
> instructs libc in case of MALLOC_CHECK_ not zero to initialize memory
> with 0xcc. The macro for_each_volume take vol->next (which is now
> 0xcccccccc) as next vol and calls free_volume again which try to free
> the memory and this cause the segmentation fault.
> 
> Example of correct behaviour is function free_options, which temporary stores the next element before freeing it.
> 
> There is currently no race condition in free before next, because this part is not multi threaded.
> 
> Questions:
> Is there a specific reason for doing that (DRBD 8.3.11 works fine with MALLOC_CHECK_) in DRBD 8.4.1 ?
> Is it planned or a bug ?

We certainly do not plan to segfault ;-)

> Patch ?

Won't be necessary.

diff --git a/user/drbdadm.h b/user/drbdadm.h
index 454fcf9..c3554d8 100644
--- a/user/drbdadm.h
+++ b/user/drbdadm.h
@@ -334,6 +334,9 @@ extern void add_setup_option(bool explicit, char *option);
 #define for_each_volume(v_,volumes_) \
 	for (v_ = volumes_; v_; v_ = v_->next)
 
+#define for_each_volume_safe(v_,tmp_,volumes_) \
+	for (v_ = volumes_; v_ && ({ tmp_ = v_->next; 1; }); v_ = tmp_)
+
 #define APPEND(LIST,ITEM) ({		      \
   typeof((LIST)) _l = (LIST);		      \
   typeof((ITEM)) _i = (ITEM);		      \
diff --git a/user/drbdadm_main.c b/user/drbdadm_main.c
index 33d9d28..ace37a7 100644
--- a/user/drbdadm_main.c
+++ b/user/drbdadm_main.c
@@ -1235,13 +1235,13 @@ static void free_volume(struct d_volume *vol)
 
 static void free_host_info(struct d_host_info *hi)
 {
-	struct d_volume *vol;
+	struct d_volume *vol, *tmp;
 
 	if (!hi)
 		return;
 
 	free_names(hi->on_hosts);
-	for_each_volume(vol, hi->volumes)
+	for_each_volume_safe(vol, tmp, hi->volumes)
 		free_volume(vol);
 	free(hi->address);
 	free(hi->address_family);


Any more segfaults to report?

Thanks,

-- 
: Lars Ellenberg
: LINBIT | Your Way to High Availability
: DRBD/HA support and consulting http://www.linbit.com