[PATCH] btrfs-progs: receive: fix a segfault that free() an err value

[PATCH] btrfs-progs: receive: fix a segfault that free() an err value

[Wang Yugui]

I noticed a segfault of 'btrfs receive'.
$ gdb
 #0  process_clone (path=0x23829d0 "after.s1.txt", offset=0, len=2097152, clone_uuid=<optimized out>,
    clone_ctransid=<optimized out>, clone_path=0x2382920 "after.s1.txt", clone_offset=0, user=0x7ffe21985ba0)
    at cmds/receive.c:793
793                     free(si->path);
(gdb) p si
$1 = (struct subvol_info *) 0xfffffffffffffffe

'si' was a ERR value here. so add the check of 'IS_ERR()' before 'free()'.

Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
---
 cmds/receive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds/receive.c b/cmds/receive.c
index d106e554..cada6343 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -789,8 +789,8 @@ static int process_clone(const char *path, u64 offset, u64 len,
 	}
 
 out:
-	if (si) {
-		free(si->path);
+	if (si && !IS_ERR(si)) {
+		if(si->path) free(si->path);
 		free(si);
 	}
 	if (clone_fd != -1)
-- 
2.36.2

Re: [PATCH] btrfs-progs: receive: fix a segfault that free() an err value

[Qu Wenruo]

On 2022/9/1 16:35, Wang Yugui wrote:
> I noticed a segfault of 'btrfs receive'.
> $ gdb
>   #0  process_clone (path=0x23829d0 "after.s1.txt", offset=0, len=2097152, clone_uuid=<optimized out>,
>      clone_ctransid=<optimized out>, clone_path=0x2382920 "after.s1.txt", clone_offset=0, user=0x7ffe21985ba0)
>      at cmds/receive.c:793
> 793                     free(si->path);
> (gdb) p si
> $1 = (struct subvol_info *) 0xfffffffffffffffe
>
> 'si' was a ERR value here. so add the check of 'IS_ERR()' before 'free()'.

The reason looks good to me, but the code doesn't follow our standard.
>
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
> ---
>   cmds/receive.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index d106e554..cada6343 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -789,8 +789,8 @@ static int process_clone(const char *path, u64 offset, u64 len,
>   	}
>
>   out:
> -	if (si) {
> -		free(si->path);
> +	if (si && !IS_ERR(si)) {
> +		if(si->path) free(si->path);

Such "if (condition) do_something();" is definitely against the common
practice.

Another thing is, that happens for the search failure for "si =
subvol_uuid_search();" line.

That's the only way @si can be a error pointer.

What about resetting @si to NULL in the else branch?

Some like this:

si = subvol_uuid_search();
if (IS_ERROR_OR_NULL(si)) {
	if (!si) {
		ret = -ENOENT;
	} else {
		ret = PTR_ERR(si);
		si = NULL;
	}
}

This removes the need to bother if @si is an error pointer or NULL at
out tag.

Thanks,
Qu
>   		free(si);
>   	}
>   	if (clone_fd != -1)

[PATCH v2] btrfs-progs: receive: fix a segfault that free() an err value

[Wang Yugui]

I noticed a segfault of 'btrfs receive'.
$ gdb
 #0  process_clone (path=0x23829d0 "after.s1.txt", offset=0, len=2097152, clone_uuid=<optimized out>,
    clone_ctransid=<optimized out>, clone_path=0x2382920 "after.s1.txt", clone_offset=0, user=0x7ffe21985ba0)
    at cmds/receive.c:793
793                     free(si->path);
(gdb) p si
$1 = (struct subvol_info *) 0xfffffffffffffffe

'si' was an ERR value. so add the check of '!IS_ERR_OR_NULL()' before 'free()'
just similar to process_snapshot().

Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
---
changes since v1:
 let the check similar to process_snapshot().

 cmds/receive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds/receive.c b/cmds/receive.c
index aec32458..bf476387 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -811,7 +811,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
 	}
 
 out:
-	if (si) {
+	if (!IS_ERR_OR_NULL(si)) {
 		free(si->path);
 		free(si);
 	}
-- 
2.36.2

Re: [PATCH v2] btrfs-progs: receive: fix a segfault that free() an err value

[David Sterba]

On Sat, Sep 03, 2022 at 12:13:27AM +0800, Wang Yugui wrote:
> I noticed a segfault of 'btrfs receive'.
> $ gdb
>  #0  process_clone (path=0x23829d0 "after.s1.txt", offset=0, len=2097152, clone_uuid=<optimized out>,
>     clone_ctransid=<optimized out>, clone_path=0x2382920 "after.s1.txt", clone_offset=0, user=0x7ffe21985ba0)
>     at cmds/receive.c:793
> 793                     free(si->path);
> (gdb) p si
> $1 = (struct subvol_info *) 0xfffffffffffffffe
> 
> 'si' was an ERR value. so add the check of '!IS_ERR_OR_NULL()' before 'free()'
> just similar to process_snapshot().
> 
> Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>

Added to devel, thanks.