| Message ID | beccdb1778697a2a46b81c85fc91c477c040397c.1617994052.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix all leaks in tests t0002-t0099: Part 1 | expand |
Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget: > From: Andrzej Hunt <ajrhunt@google.com> > > common_prefix() returns a new string, which we store in max_prefix - > this string needs to be freed to avoid a leak. This leak is happening > in cmd_ls_files, hence is of no real consequence - an UNLEAK would be > just as good, but we might as well free the string properly. > > Leak found while running t0002, see output below: > > Direct leak of 8 byte(s) in 1 object(s) allocated from: > #0 0x49a85d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 > #1 0x9ab1b4 in do_xmalloc wrapper.c:41:8 > #2 0x9ab248 in do_xmallocz wrapper.c:75:8 > #3 0x9ab22a in xmallocz wrapper.c:83:9 > #4 0x9ab2d7 in xmemdupz wrapper.c:99:16 > #5 0x78d6a4 in common_prefix dir.c:191:15 > #6 0x5aca48 in cmd_ls_files builtin/ls-files.c:669:16 > #7 0x4cd92d in run_builtin git.c:453:11 > #8 0x4cb5fa in handle_builtin git.c:704:3 > #9 0x4ccf57 in run_argv git.c:771:4 > #10 0x4caf49 in cmd_main git.c:902:19 > #11 0x69ce2e in main common-main.c:52:11 > #12 0x7f64d4d94349 in __libc_start_main (/lib64/libc.so.6+0x24349) > > Signed-off-by: Andrzej Hunt <ajrhunt@google.com> > --- > builtin/ls-files.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > index 60a2913a01e9..53e20bbf9cce 100644 > --- a/builtin/ls-files.c > +++ b/builtin/ls-files.c > @@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) > } > > dir_clear(&dir); > + free((void *)max_prefix); This cast is necessary to ignore the const attribute of the pointer. It's scary, but safe here because this function owns the referenced object. I think the promise to not modify the string given at the top of the function is not worth having to take back that promise forcefully at the end to dispose of it. Determining the correctness of this cast requires reading the whole function. Removing the const from the declaration (and the cast) would improve readability overall. Thoughts? > return 0; > } >
On 10/04/2021 10:12, René Scharfe wrote: > Am 09.04.21 um 20:47 schrieb Andrzej Hunt via GitGitGadget: >> From: Andrzej Hunt <ajrhunt@google.com> >> diff --git a/builtin/ls-files.c b/builtin/ls-files.c >> index 60a2913a01e9..53e20bbf9cce 100644 >> --- a/builtin/ls-files.c >> +++ b/builtin/ls-files.c >> @@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) >> } >> >> dir_clear(&dir); >> + free((void *)max_prefix); > > This cast is necessary to ignore the const attribute of the pointer. > It's scary, but safe here because this function owns the referenced > object. > > I think the promise to not modify the string given at the top of the > function is not worth having to take back that promise forcefully at > the end to dispose of it. Determining the correctness of this cast > requires reading the whole function. Removing the const from the > declaration (and the cast) would improve readability overall. Thoughts? I agree - I'll change this in V2 V2. In fact, Peff already given the following explanation for why non-const is preferred in this scenario on a previous patch of mine (which I failed to heed when preparing this patch): > If a variable is meant to take ownership of memory, our usual > convention is to not declare it as "const"." https://lore.kernel.org/git/YEZ0jLppB9wOg%2Faf@coredump.intra.peff.net/ > >> return 0; >> } >> >
diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 60a2913a01e9..53e20bbf9cce 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -781,5 +781,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) } dir_clear(&dir); + free((void *)max_prefix); return 0; }