Re: [PATCH] Fix a use-after-free in the volumes code.

[Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>]

Hi Adam,

(2014/06/14 8:43), Adam Buchbinder wrote:
> When a struct btrfs_fs_devices was being torn down by
> btrfs_close_devices(), there was an invalidated pointer in the global
> list fs_uuids which still pointed to it; if a device was closed and
> then reopened (which btrfs-convert does), freed memory would be
> accessed.

It looks good to me.

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

I confirmed ...

 - btrfs_fs_devices is linked to fs_uuids in device_list_add().
 - However, there is no list_del(btrfs_fs_devices) from
   fs_uuids at all.
 - There is at least one use-after-free of btrfs_fs_devices
   as you said (btrfs-convert).

P.S.
I suggest you to add "btrfs-progs: " or "btrfs:" tag to mail subject
from the next time.

Thanks,
Satoru

> 
> This was found using ThreadSanitizer (pretty much doing what
> AddressSanitizer would, but not exiting after the first failure).
> To reproduce, build with -fsanitize=thread and run 'make test'.
> Representative output is below.
> 
> This change makes the current tests TSan-clean.
> 
> WARNING: ThreadSanitizer: heap-use-after-free (pid=29161)
>    Read of size 8 at 0x7d180000eee0 by main thread:
>      #0 memcmp ??:0
>      #1 find_fsid .../volumes.c:81
>      #2 device_list_add .../volumes.c:95
>      #3 btrfs_scan_one_device .../volumes.c:259
>      #4 btrfs_scan_fs_devices .../disk-io.c:1002
>      #5 __open_ctree_fd .../disk-io.c:1090
>      #6 open_ctree_fd .../disk-io.c:1191
>      #7 do_convert .../btrfs-convert.c:2317
>      #8 main .../btrfs-convert.c:2745
> 
>    Previous write of size 8 at 0x7d180000eee0 by main thread:
>      #0 free ??:0
>      #1 btrfs_close_devices .../volumes.c:191
>      #2 close_ctree .../disk-io.c:1401
>      #3 do_convert .../btrfs-convert.c:2300
>      #4 main .../btrfs-convert.c:2745
> 
>    Location is heap block of size 96 at 0x7d180000eee0 allocated by main thread:
>      #0 calloc ??:0 (exe+0x00000002acc6)
>      #1 device_list_add .../volumes.c:97
>      #2 btrfs_scan_one_device .../volumes.c:259
>      #3 btrfs_scan_fs_devices .../disk-io.c:1002
>      #4 __open_ctree_fd .../disk-io.c:1090
>      #5 open_ctree_fd .../disk-io.c:1191
>      #6 do_convert .../btrfs-convert.c:2256
>      #7 main .../btrfs-convert.c:2745
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
> ---
>   volumes.c | 1 +
>   volumes.h | 2 +-
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/volumes.c b/volumes.c
> index a61928c..36f6050 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -188,6 +188,7 @@ again:
>   		goto again;
>   	}
>   
> +	list_del(&fs_devices->list);
>   	free(fs_devices);
>   	return 0;
>   }
> diff --git a/volumes.h b/volumes.h
> index b1ff3d0..2e960b5 100644
> --- a/volumes.h
> +++ b/volumes.h
> @@ -67,7 +67,7 @@ struct btrfs_device {
>   struct btrfs_fs_devices {
>   	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
>   
> -	/* the device with this id has the most recent coyp of the super */
> +	/* the device with this id has the most recent copy of the super */
>   	u64 latest_devid;
>   	u64 latest_trans;
>   	u64 lowest_devid;
>