| 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 |
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 --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)
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(-)