[PATCH] btrfs-progs: super-recover: fix double free fs_devices memory

[PATCH] btrfs-progs: super-recover: fix double free fs_devices memory

[robbieko]

From: Robbie Ko <robbieko@synology.com>

super-recover collects btrfs devices information using existed
functions scan_one_devices().

Problem is fs_devices is freed twice. One in __open_ctree_fd() when
error happens and the other in btrfs_close_devices(recover.fs_devices)
when root is NULL.

Commit "30fd6f2e92695c355c8f76b8887cd4fade60cdac" add force-close
all opened device before program exit, to avoid memory leak in all
btrfs sub-command.
Therefore, there is an unnecessary freed of fs_devices in
btrfs_recover_superblocks.

Fix this problem by remove unnecessary freed of fs_devices.

Signed-off-by: Robbie Ko <robbieko@synology.com>
---
 super-recover.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/super-recover.c b/super-recover.c
index 880fd77..86b3df9 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -292,9 +292,6 @@ int btrfs_recover_superblocks(const char *dname,
 no_recover:
 	recover_err_str(ret);
 	free_recover_superblock(&recover);
-	/* check if we have freed fs_devices in close_ctree() */
-	if (!root)
-		btrfs_close_devices(recover.fs_devices);
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH] btrfs-progs: super-recover: fix double free fs_devices memory

[David Sterba]

On Fri, Oct 12, 2018 at 04:25:23PM +0800, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
> 
> super-recover collects btrfs devices information using existed
> functions scan_one_devices().
> 
> Problem is fs_devices is freed twice. One in __open_ctree_fd() when
> error happens and the other in btrfs_close_devices(recover.fs_devices)
> when root is NULL.
> 
> Commit "30fd6f2e92695c355c8f76b8887cd4fade60cdac" add force-close
> all opened device before program exit, to avoid memory leak in all
> btrfs sub-command.
> Therefore, there is an unnecessary freed of fs_devices in
> btrfs_recover_superblocks.
> 
> Fix this problem by remove unnecessary freed of fs_devices.
> 
> Signed-off-by: Robbie Ko <robbieko@synology.com>

The fix for the double free has been in the devel branch, but thanks
anyway.

"btrfs rescue super-recover" memory corruption

[Eric Sandeen]

This:

# truncate --size=8g
# dd if=/dev/zero of=file conv=notrunc  bs=4 seek=16384 count=1
# valgrind ./btrfs rescue super-recover file -v

yields:

==4604== Memcheck, a memory error detector
==4604== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==4604== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==4604== Command: ./btrfs rescue super-recover file -v
==4604== 
All Devices:
	Device: id = 1, name = file

Before Recovering:
	[All good supers]:
		device name = file
		superblock bytenr = 67108864

	[All bad supers]:
		device name = file
		superblock bytenr = 65536


Make sure this is a btrfs disk otherwise the tool will destroy other fs, Are you sure? [y/N]: y
Recovered bad superblocks successful
==4604== Invalid read of size 8
==4604==    at 0x426B55: btrfs_recover_superblocks (list.h:204)
==4604==    by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604==    by 0x40420A: handle_command_group (btrfs.c:145)
==4604==    by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604==    by 0x404199: main (btrfs.c:247)
==4604==  Address 0x4c250b0 is 48 bytes inside a block of size 96 free'd
==4604==    at 0x4A063F0: free (vg_replace_malloc.c:446)
==4604==    by 0x43C77E: btrfs_close_devices (volumes.c:196)
==4604==    by 0x42F5D1: close_ctree (disk-io.c:1404)
==4604==    by 0x426A85: btrfs_recover_superblocks (super-recover.c:340)
==4604==    by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604==    by 0x40420A: handle_command_group (btrfs.c:145)
==4604==    by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604==    by 0x404199: main (btrfs.c:247)
==4604== 
==4604== Invalid free() / delete / delete[] / realloc()
==4604==    at 0x4A063F0: free (vg_replace_malloc.c:446)
==4604==    by 0x426B9E: btrfs_recover_superblocks (super-recover.c:85)
==4604==    by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604==    by 0x40420A: handle_command_group (btrfs.c:145)
==4604==    by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604==    by 0x404199: main (btrfs.c:247)
==4604==  Address 0x4c25080 is 0 bytes inside a block of size 96 free'd
==4604==    at 0x4A063F0: free (vg_replace_malloc.c:446)
==4604==    by 0x43C77E: btrfs_close_devices (volumes.c:196)
==4604==    by 0x42F5D1: close_ctree (disk-io.c:1404)
==4604==    by 0x426A85: btrfs_recover_superblocks (super-recover.c:340)
==4604==    by 0x421C79: cmd_super_recover (cmds-rescue.c:148)
==4604==    by 0x40420A: handle_command_group (btrfs.c:145)
==4604==    by 0x421B54: cmd_rescue (cmds-rescue.c:162)
==4604==    by 0x404199: main (btrfs.c:247)
==4604== 
==4604== 
==4604== HEAP SUMMARY:
==4604==     in use at exit: 0 bytes in 0 blocks
==4604==   total heap usage: 72 allocs, 73 frees, 140,384 bytes allocated
==4604== 
==4604== All heap blocks were freed -- no leaks are possible
==4604== 
==4604== For counts of detected and suppressed errors, rerun with: -v
==4604== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 6 from 6)

i.e. I think we are double freeing memory:

        close_ctree(root); // <-- here
no_recover:
        recover_err_str(ret);
        free_recover_superblock(&recover); // <-- and here

I can't really work out what all this is all doing, but maybe the fix is obvious
to Wang Shilong (who wrote the original code)?

Thanks,
-Eric

[PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory

[Wang Shilong]

super-recover collects btrfs devices infomation using existed
functions scan_one_devices().

Problem is fs_devices is freed twice in close_ctree() and
free_recover_superblock() for super correction path.

Fix this problem by checking whether fs_devices memory
have been freed before we free it.

Cc: Eric Sandeen <sandeen@redhat.com>
Cc: Chris Murphy <lists@colorremedies.com>
Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>
---
 super-recover.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/super-recover.c b/super-recover.c
index 767de4b..419b86a 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -69,21 +69,11 @@ void init_recover_superblock(struct btrfs_recover_superblock *recover)
 static
 void free_recover_superblock(struct btrfs_recover_superblock *recover)
 {
-	struct btrfs_device *device;
 	struct super_block_record *record;
 
 	if (!recover->fs_devices)
 		return;
 
-	while (!list_empty(&recover->fs_devices->devices)) {
-		device = list_entry(recover->fs_devices->devices.next,
-				struct btrfs_device, dev_list);
-		list_del_init(&device->dev_list);
-		free(device->name);
-		free(device);
-	}
-	free(recover->fs_devices);
-
 	while (!list_empty(&recover->good_supers)) {
 		record = list_entry(recover->good_supers.next,
 				struct super_block_record, list);
@@ -341,6 +331,9 @@ int btrfs_recover_superblocks(const char *dname,
 no_recover:
 	recover_err_str(ret);
 	free_recover_superblock(&recover);
+	/* check if we have freed fs_deivces in close_ctree() */
+	if (!root)
+		btrfs_close_devices(recover.fs_devices);
 	return ret;
 }
 
-- 
1.9.3

Re: [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory

[Gui Hecheng]

On Thu, 2014-09-18 at 05:01 -0400, Wang Shilong wrote:
> super-recover collects btrfs devices infomation using existed
> functions scan_one_devices().
> 
> Problem is fs_devices is freed twice in close_ctree() and
> free_recover_superblock() for super correction path.
> 
> Fix this problem by checking whether fs_devices memory
> have been freed before we free it.
> 
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>
> ---
>  super-recover.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/super-recover.c b/super-recover.c
> index 767de4b..419b86a 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -69,21 +69,11 @@ void init_recover_superblock(struct btrfs_recover_superblock *recover)
>  static
>  void free_recover_superblock(struct btrfs_recover_superblock *recover)
>  {
> -	struct btrfs_device *device;
>  	struct super_block_record *record;
>  
>  	if (!recover->fs_devices)
>  		return;
>  
> -	while (!list_empty(&recover->fs_devices->devices)) {
> -		device = list_entry(recover->fs_devices->devices.next,
> -				struct btrfs_device, dev_list);
> -		list_del_init(&device->dev_list);
> -		free(device->name);
> -		free(device);
> -	}
> -	free(recover->fs_devices);
> -
>  	while (!list_empty(&recover->good_supers)) {
>  		record = list_entry(recover->good_supers.next,
>  				struct super_block_record, list);
> @@ -341,6 +331,9 @@ int btrfs_recover_superblocks(const char *dname,
>  no_recover:
>  	recover_err_str(ret);
>  	free_recover_superblock(&recover);
> +	/* check if we have freed fs_deivces in close_ctree() */
> +	if (!root)
> +		btrfs_close_devices(recover.fs_devices);
>  	return ret;
>  }
>  

nice catch! "+20, recorded". ^_^

Re: [PATCH] Btrfs-progs: super-recover: fix double free fs_devices memory

[Eric Sandeen]

On 9/18/14 4:01 AM, Wang Shilong wrote:
> super-recover collects btrfs devices infomation using existed
> functions scan_one_devices().
> 
> Problem is fs_devices is freed twice in close_ctree() and
> free_recover_superblock() for super correction path.
> 
> Fix this problem by checking whether fs_devices memory
> have been freed before we free it.
> 
> Cc: Eric Sandeen <sandeen@redhat.com>
> Cc: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Wang Shilong <wangshilong1991@gmail.com>

That does seem to fix the testcase.  Thanks!

Acked-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  super-recover.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/super-recover.c b/super-recover.c
> index 767de4b..419b86a 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -69,21 +69,11 @@ void init_recover_superblock(struct btrfs_recover_superblock *recover)
>  static
>  void free_recover_superblock(struct btrfs_recover_superblock *recover)
>  {
> -	struct btrfs_device *device;
>  	struct super_block_record *record;
>  
>  	if (!recover->fs_devices)
>  		return;
>  
> -	while (!list_empty(&recover->fs_devices->devices)) {
> -		device = list_entry(recover->fs_devices->devices.next,
> -				struct btrfs_device, dev_list);
> -		list_del_init(&device->dev_list);
> -		free(device->name);
> -		free(device);
> -	}
> -	free(recover->fs_devices);
> -
>  	while (!list_empty(&recover->good_supers)) {
>  		record = list_entry(recover->good_supers.next,
>  				struct super_block_record, list);
> @@ -341,6 +331,9 @@ int btrfs_recover_superblocks(const char *dname,
>  no_recover:
>  	recover_err_str(ret);
>  	free_recover_superblock(&recover);
> +	/* check if we have freed fs_deivces in close_ctree() */
> +	if (!root)
> +		btrfs_close_devices(recover.fs_devices);
>  	return ret;
>  }
>  
>