| Message ID | patch-1.3-3d0d7a8e8b5-20210630T140339Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle() | expand |
On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > Fix a memory leak from the prefix_filename() function introduced with > its use in 3b754eedd5 (bundle: use prefix_filename with bundle path, > 2017-03-20). > > As noted in that commit the leak was intentional as a part of being > sloppy about freeing resources just before we exit, I'm changing this > because I'll be fixing other memory leaks in the bundle API (including > the library version) in subsequent commits. It's easier to reason > about those fixes if valgrind runs cleanly at the end without any > leaks whatsoever. Thanks, this looks good to me. One thing, though... > An earlier version of this change went out of its way to not leak > memory on the die() codepaths here, but that was deemed too verbose to > worry about in a built-in that's dying anyway. The only reason we'd > need that is to appease a mode like SANITIZE=leak within the scope of > an entire test file. Obviously you changed this as I asked, but this final sentence makes me think we're not on the same page with respect to die(). I don't think any kind of mode matters here. When we call die(), whatever we have on the stack is _not_ a leak, by LSan's or valgrind's standards. Because we still have access to those bytes. And nor can we ever get rid of such cases. If we ever do: void foo(const char *str) { char *x = xstrdup(str); bar(x); free(x); } void bar(const char *x) { if (!strcmp(x, "foo")) die("whatever"); } Then "x" will always still be allocated when we die(). We cannot free it in bar(), where it is read-only. We cannot free it in foo() before we call bar(), because it is needed there. But control never returns to the free() statement. So this code is perfectly fine and unavoidable. In the case you were touching it was foo() that was calling die() directly, so we could work around it with some conditionals. But from the leak-checker's perspective the two cases are the same. -Peff
On Wed, Jun 30 2021, Jeff King wrote: > On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Fix a memory leak from the prefix_filename() function introduced with >> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path, >> 2017-03-20). >> >> As noted in that commit the leak was intentional as a part of being >> sloppy about freeing resources just before we exit, I'm changing this >> because I'll be fixing other memory leaks in the bundle API (including >> the library version) in subsequent commits. It's easier to reason >> about those fixes if valgrind runs cleanly at the end without any >> leaks whatsoever. > > Thanks, this looks good to me. > > One thing, though... > >> An earlier version of this change went out of its way to not leak >> memory on the die() codepaths here, but that was deemed too verbose to >> worry about in a built-in that's dying anyway. The only reason we'd >> need that is to appease a mode like SANITIZE=leak within the scope of >> an entire test file. > > Obviously you changed this as I asked, but this final sentence makes me > think we're not on the same page with respect to die(). I don't think > any kind of mode matters here. When we call die(), whatever we have on > the stack is _not_ a leak, by LSan's or valgrind's standards. Because we > still have access to those bytes. And nor can we ever get rid of such > cases. If we ever do: > > void foo(const char *str) > { > char *x = xstrdup(str); > bar(x); > free(x); > } > > void bar(const char *x) > { > if (!strcmp(x, "foo")) > die("whatever"); > } > > Then "x" will always still be allocated when we die(). We cannot free it > in bar(), where it is read-only. We cannot free it in foo() before we > call bar(), because it is needed there. But control never returns to the > free() statement. > > So this code is perfectly fine and unavoidable. In the case you were > touching it was foo() that was calling die() directly, so we could work > around it with some conditionals. But from the leak-checker's > perspective the two cases are the same. I've got you to blame for introducing SANITIZE=*. Now I've got these leak checkers all mixed up :) Yes you're right, FWIW I (re-)wrote this commit message just before sending and should have said "a heap leak checker" instead of "SANITIZE=leak", I really meant valgrind. I originally ended with the "we are about to die" tracking because I was tracing things with valgrind, which does complain about this sort of thing. I.e.: 24 bytes in 1 blocks are still reachable in loss record 8 of 21 at 0x48356AF: malloc (vg_replace_malloc.c:298) by 0x4837DE7: realloc (vg_replace_malloc.c:826) by 0x3C06F1: xrealloc (wrapper.c:126) by 0x380EC9: strbuf_grow (strbuf.c:98) by 0x381A14: strbuf_add (strbuf.c:297) by 0x20ADC5: strbuf_addstr (strbuf.h:304) by 0x20B66D: prefix_filename (abspath.c:277) by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55) by 0x13D565: cmd_bundle_unbundle (bundle.c:170) by 0x13D829: cmd_bundle (bundle.c:214) by 0x1279F4: run_builtin (git.c:461) by 0x127DFB: handle_builtin (git.c:714) Re what I mentioned/asked in https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was experimenting with doing leak checking in the tests. In this case we have 21 in total under --show-leak-kinds=all (and it was 20 in v2 of this series). I've found that only including the file tho builtin is in cuts down on it to a meaningful set of leaks. I.e. to throw out everything not including /\bbundle\.c:/. We leak in a lot of things we call from common-main.c, git.c, exec-cmd.c etc. Maybe if we do end up with a test mode for this we shouldn't care about checkers like valgrind and only cater to the SANITIZE=leak mode. I'm still partial to the idea that we'll get the most win out of something that we can run in the tests by default, i.e. we'll only need to have a valgrind on the system & have someone run "make test" to run a (limited set of) tests with this. Whereas SANITIZE=leak is always a dev-only feature people may not have on all the time.
On Wed, Jun 30, 2021 at 08:00:50PM +0200, Ævar Arnfjörð Bjarmason wrote: > > So this code is perfectly fine and unavoidable. In the case you were > > touching it was foo() that was calling die() directly, so we could work > > around it with some conditionals. But from the leak-checker's > > perspective the two cases are the same. > > I've got you to blame for introducing SANITIZE=*. Now I've got these > leak checkers all mixed up :) > > Yes you're right, FWIW I (re-)wrote this commit message just before > sending and should have said "a heap leak checker" instead of > "SANITIZE=leak", I really meant valgrind. Ah, OK, that makes more sense. I think the distinction here isn't "heap leak checker" (since valgrind does still look at the whole memory space to find references), but rather the treatment of "still reachable" items. I think LSan/ASan ignore these entirely. > I originally ended with the "we are about to die" tracking because I was > tracing things with valgrind, which does complain about this sort of > thing. I.e.: > > 24 bytes in 1 blocks are still reachable in loss record 8 of 21 > at 0x48356AF: malloc (vg_replace_malloc.c:298) > by 0x4837DE7: realloc (vg_replace_malloc.c:826) > by 0x3C06F1: xrealloc (wrapper.c:126) > by 0x380EC9: strbuf_grow (strbuf.c:98) > by 0x381A14: strbuf_add (strbuf.c:297) > by 0x20ADC5: strbuf_addstr (strbuf.h:304) > by 0x20B66D: prefix_filename (abspath.c:277) > by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55) > by 0x13D565: cmd_bundle_unbundle (bundle.c:170) > by 0x13D829: cmd_bundle (bundle.c:214) > by 0x1279F4: run_builtin (git.c:461) > by 0x127DFB: handle_builtin (git.c:714) > > Re what I mentioned/asked in > https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was > experimenting with doing leak checking in the tests. > > In this case we have 21 in total under --show-leak-kinds=all (and it was > 20 in v2 of this series). IMHO these "still reachable" leaks are not interesting. They'll fall into one of two categories: - allocations still on the stack when we exit() without unwinding. These are always uninteresting, since by definition we are exiting the process. - globals that hang on to allocations (which will still be present even if we exit the program by returning up through main()). Most of these will be items we intend to last for the program length (e.g., fields in the_repository). It's _possible_ that some of these could be interesting. E.g., a program might have two phases: in the first, we rely on some subsystem which uses global variables to cache some data (say, parsed config values in remote.c or userdiff.c), and in the second phase we no longer use that subsystem. We could be instructing the subsystem to free up the memory after the first phase. But in practice this is awkward, because the program-level code doesn't know about subsystem allocations (or even which subsystems might have been recursively triggered). And while still-reachable tracking could be used to find opportunities like this, I think there's a better approach to finding these. If subsystems avoid global-variable caches and instead stick their allocations into context structs, then the memory is associated with those structs. So for example, if userdiff attached its storage to a diff_options, which is attached to a rev_info, then any "leaking" is all predicated on the rev_info (which the main program either cleans up, or annotates with UNLEAK). > Maybe if we do end up with a test mode for this we shouldn't care about > checkers like valgrind and only cater to the SANITIZE=leak mode. I do find SANITIZE=leak to be a more useful tool in general, but I'm not at all against using valgrind if people find it convenient. I just think "reachable" leaks are not interesting enough to be part of what we're searching for when we run the test suite. > I'm still partial to the idea that we'll get the most win out of > something that we can run in the tests by default, i.e. we'll only need > to have a valgrind on the system & have someone run "make test" to run a > (limited set of) tests with this. > > Whereas SANITIZE=leak is always a dev-only feature people may not have > on all the time. That is exactly why I think "SANITIZE=leak" is better: it is easier for people to run. valgrind is _extremely_ slow. It's also not available everywhere; ASan/LSan aren't either, but they're pretty standard in clang and gcc these days. It is nice that valgrind can run on an un-instrumented binary, but I sort of assume that anybody running "make test" will be able to build the binary, since "make" is going to want to do that. I.e., I think "make SANITIZE=leak test" is already doing what we want (we just need to further annotate known-failures). -Peff
diff --git a/builtin/bundle.c b/builtin/bundle.c index ea6948110b0..15e2bd61965 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc, const char* prefix, const char * const usagestr[], const struct option options[], - const char **bundle_file) { + char **bundle_file) { int newargc; newargc = parse_options(argc, argv, NULL, options, usagestr, PARSE_OPT_STOP_AT_NON_OPTION); @@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) { int progress = isatty(STDERR_FILENO); struct strvec pack_opts; int version = -1; - + int ret; struct option options[] = { OPT_SET_INT('q', "quiet", &progress, N_("do not show progress meter"), 0), @@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) { N_("specify bundle format version")), OPT_END() }; - const char* bundle_file; + char *bundle_file; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_create_usage, options, &bundle_file); @@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) { if (!startup_info->have_repository) die(_("Need a repository to create a bundle.")); - return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version); + ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version); + free(bundle_file); + return ret; } static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) { struct bundle_header header; int bundle_fd = -1; int quiet = 0; - + int ret; struct option options[] = { OPT_BOOL('q', "quiet", &quiet, N_("do not show bundle details")), OPT_END() }; - const char* bundle_file; + char *bundle_file; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_verify_usage, options, &bundle_file); /* bundle internals use argv[1] as further parameters */ memset(&header, 0, sizeof(header)); - if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) - return 1; + if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) { + ret = 1; + goto cleanup; + } close(bundle_fd); - if (verify_bundle(the_repository, &header, !quiet)) - return 1; + if (verify_bundle(the_repository, &header, !quiet)) { + ret = 1; + goto cleanup; + } + fprintf(stderr, _("%s is okay\n"), bundle_file); - return 0; + ret = 0; +cleanup: + free(bundle_file); + return ret; } static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) { struct bundle_header header; int bundle_fd = -1; - + int ret; struct option options[] = { OPT_END() }; - const char* bundle_file; + char *bundle_file; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_list_heads_usage, options, &bundle_file); /* bundle internals use argv[1] as further parameters */ memset(&header, 0, sizeof(header)); - if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) - return 1; + if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) { + ret = 1; + goto cleanup; + } close(bundle_fd); - return !!list_bundle_refs(&header, argc, argv); + ret = !!list_bundle_refs(&header, argc, argv); +cleanup: + free(bundle_file); + return ret; } static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) { struct bundle_header header; int bundle_fd = -1; - + int ret; struct option options[] = { OPT_END() }; - const char* bundle_file; + char *bundle_file; argc = parse_options_cmd_bundle(argc, argv, prefix, builtin_bundle_unbundle_usage, options, &bundle_file); /* bundle internals use argv[1] as further parameters */ memset(&header, 0, sizeof(header)); - if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) - return 1; + if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) { + ret = 1; + goto cleanup; + } if (!startup_info->have_repository) die(_("Need a repository to unbundle.")); - return !!unbundle(the_repository, &header, bundle_fd, 0) || + ret = !!unbundle(the_repository, &header, bundle_fd, 0) || list_bundle_refs(&header, argc, argv); +cleanup: + free(bundle_file); + return ret; } int cmd_bundle(int argc, const char **argv, const char *prefix)
Fix a memory leak from the prefix_filename() function introduced with its use in 3b754eedd5 (bundle: use prefix_filename with bundle path, 2017-03-20). As noted in that commit the leak was intentional as a part of being sloppy about freeing resources just before we exit, I'm changing this because I'll be fixing other memory leaks in the bundle API (including the library version) in subsequent commits. It's easier to reason about those fixes if valgrind runs cleanly at the end without any leaks whatsoever. An earlier version of this change went out of its way to not leak memory on the die() codepaths here, but that was deemed too verbose to worry about in a built-in that's dying anyway. The only reason we'd need that is to appease a mode like SANITIZE=leak within the scope of an entire test file. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 21 deletions(-)