[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()
From:       Ævar Arnfjörð Bjarmason <avarab () gmail ! com>
Date:       2021-06-30 18:00:50
Message-ID: 87v95vdxrc.fsf () evledraar ! gmail ! com
[Download RAW message or body]


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.

[prev in list] [next in list] [prev in thread] [next in thread]