diff mbox series

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

Message ID 20220901083554.40166-1-wangyugui@e16-tech.com (mailing list archive)
State New, archived
Headers show
Series btrfs-progs: receive: fix a segfault that free() an err value | expand

Commit Message

Wang Yugui Sept. 1, 2022, 8:35 a.m. UTC
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(-)

Comments

Qu Wenruo Sept. 1, 2022, 9:13 a.m. UTC | #1
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)
diff mbox series

Patch

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)