git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] pack-objects: lazily set up "struct rev_info", don't leak

From
Derrick Stolee <derrickstolee@github.com>
Date
Mar 25, 2022, 16:41 UTC
Message-ID
<8d368240-dae5-7a66-6c0c-9e0a960ca34c@github.com>
In-Reply-To
<220325.86r16qkodl.gmgdl@evledraar.gmail.com>
On 3/25/2022 12:00 PM, Ævar Arnfjörð Bjarmason wrote:
Show 62 quoted lines
> 
> On Fri, Mar 25 2022, Derrick Stolee wrote:
> 
>> On 3/25/2022 10:25 AM, Ævar Arnfjörð Bjarmason wrote:
>>> In the preceding [1] (pack-objects: move revs out of
>>> get_object_list(), 2022-03-22) the "repo_init_revisions()" was moved
>>> to cmd_pack_objects() so that it unconditionally took place for all
>>> invocations of "git pack-objects".
>>>
>>> We'd thus start leaking memory, which is easily reproduced in
>>> e.g. git.git by feeding e83c5163316 (Initial revision of "git", the
>>> information manager from hell, 2005-04-07) to "git pack-objects";
>>>
>>>     $ echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial
>>>     [...]
>>> 	==19130==ERROR: LeakSanitizer: detected memory leaks
>>>
>>> 	Direct leak of 7120 byte(s) in 1 object(s) allocated from:
>>> 	    #0 0x455308 in __interceptor_malloc (/home/avar/g/git/git+0x455308)
>>> 	    #1 0x75b399 in do_xmalloc /home/avar/g/git/wrapper.c:41:8
>>> 	    #2 0x75b356 in xmalloc /home/avar/g/git/wrapper.c:62:9
>>> 	    #3 0x5d7609 in prep_parse_options /home/avar/g/git/diff.c:5647:2
>>> 	    #4 0x5d415a in repo_diff_setup /home/avar/g/git/diff.c:4621:2
>>> 	    #5 0x6dffbb in repo_init_revisions /home/avar/g/git/revision.c:1853:2
>>> 	    #6 0x4f599d in cmd_pack_objects /home/avar/g/git/builtin/pack-objects.c:3980:2
>>> 	    #7 0x4592ca in run_builtin /home/avar/g/git/git.c:465:11
>>> 	    #8 0x457d81 in handle_builtin /home/avar/g/git/git.c:718:3
>>> 	    #9 0x458ca5 in run_argv /home/avar/g/git/git.c:785:4
>>> 	    #10 0x457b40 in cmd_main /home/avar/g/git/git.c:916:19
>>> 	    #11 0x562259 in main /home/avar/g/git/common-main.c:56:11
>>> 	    #12 0x7fce792ac7ec in __libc_start_main csu/../csu/libc-start.c:332:16
>>> 	    #13 0x4300f9 in _start (/home/avar/g/git/git+0x4300f9)
>>>
>>> 	SUMMARY: LeakSanitizer: 7120 byte(s) leaked in 1 allocation(s).
>>> 	Aborted
>>>
>>> Narrowly fixing that commit would have been easy, just add call
>>> repo_init_revisions() right before get_object_list(), which is
>>> effectively what was done before that commit.
>>>
>>> But an unstated constraint when setting it up early is that it was
>>> needed for the subsequent [2] (pack-objects: parse --filter directly
>>> into revs.filter, 2022-03-22), i.e. we might have a --filter
>>> command-line option, and need to either have the "struct rev_info"
>>> setup when we encounter that option, or later.
>>>
>>> Let's just change the control flow so that we'll instead set up the
>>> "struct rev_info" only when we need it. Doing so leads to a bit more
>>> verbosity, but it's a lot clearer what we're doing and why.
>>
>> This makes sense.
>>
>>> We could furthermore combine the two get_object_list() invocations
>>> here by having repo_init_revisions() invoked on &pfd.revs, but I think
>>> clearly separating the two makes the flow clearer. Likewise
>>> redundantly but explicitly (i.e. redundant v.s. a "{ 0 }") "0" to
>>> "have_revs" early in cmd_pack_objects().
>>
>> I disagree, especially when you later want to make sure we free
>> the data from revs using your release_revisions().
> 
> Because we'll need two release_revisions() instead of one?

And it would be easy to miss one of the two if you are only testing certain paths with leak-check on.

Show 8 quoted lines
>>> This does add the future constraint to opt_parse_list_objects_filter()
>>> that we'll need to adjust this wrapper code if it looks at any other
>>> value of the "struct option" than the "value" member.
>>
>> So we are coupling ourselves to the implementation of this method.
> 
> Sure, but aren't all OPT_* where the macro is providing some custom
> callback coupling themselves to how it's implemented?

No, I mean that the callback function you are making here is coupling itself to the existing callback function in a novel way.

I tried to find another example of a nested callback, but I didn't even find another instance of creating a new 'struct option'.

Show 19 quoted lines
>>> But that regression should be relatively easy to spot. I'm
>>> intentionally not initializing the "struct wrap" with e.g. "{ 0 }" so
>>> that various memory sanity checkers would spot that, we just
>>> initialize the "value" in po_filter_cb(). By doing this e.g. we'll die
>>> on e.g. this test if we were to use another member of "opt" in
>>> opt_parse_list_objects_filter()>
>>
>> So you are using uninitialized memory as a way to discover any
>> necessary changes to that coupling. I'm not sure this is super-safe
>> because we don't necessarily run memory checkers during CI builds.
>>
>> I'd rather have a consistently initialized chunk of data that would
>> behave predictably (and hopefully we'd discover it is behaving
>> incorrectly with that predictable behavior).
> 
> I'd like to keep this as-is, i.e. if you zero it out it might also
> behave unpredictably or even in undefined ways (e.g. a NULL ptr
> dereference). Likewise in my version, but it has the benefit of being
> caught by some tooling we have.
I guess by "predictable" I mean "well-defined" or "deterministic".

I don't want the build options to change how this works (for instance, debug builds sometimes initialize data to zero).

>> +struct rev_info_maybe_empty {
>> +	int has_revs;
>> +	struct rev_info revs;
>> +};

Thinking about this a second time, perhaps it would be best to add an "unsigned initialized:1;" to struct rev_info so we can look at such a struct and know whether or not repo_init_revisions() has been run or not. Avoids the custom struct and unifies a few things.

In particular, release_revisions() could choose to do nothing if revs->initialized is false.

Further, a second repo_init_revisions() could do nothing if revs->initialized is true. This allows us to safely "re-init" without our own "if (has_revs)" checks...

Show 7 quoted lines
>> 	else {
>> 		if (!pfd.have_revs) {
>> 			repo_init_revisions(the_repository, &pfd.revs, NULL);
>> 			pfd.have_revs = 1;
>> 		}
>> 		get_object_list(&pfd.revs, rp.nr, rp.v);
>> 	}
...making this just
	else {
		repo_init_revisions(the_repository, &revs, NULL);
		get_object_list(&revs. rp.nr, rp.v);
	}
Show 11 quoted lines
> Conceptually I think the saving of that one line isn't worth it to the
> reader.
> 
> Then you'd need to read up and see exactly how pfd.revs gets mutated,
> and its callback etc., only to see we're just doing this to save
> ourselves a variable deceleration and a call to release_revisions().
> 
>> and then later you can add
>>
>> 	if (pfd.have_revs)
>> 		release_revisions(&pfd.revs);
And this would just be
	release_revisions(&revs);
>> to clear the memory in exactly one place.
> 
> Yeah, it would work. I'd just prefer control flow that's trivial to
> reason about over saving a couple of lines here.

I think having multiple revision things that can live at different levels (one embedded in a custom struct, one not) is not trivial to reason about. If we change the "is this initialized" indicator to be within 'struct rev_info', then this gets simpler.

It seems to me that it is easier to track a single struct and release the memory in one place based on its lifespan.

Thanks, -Stolee

Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 16 of 25 in “Partial bundle follow ups”
  1. 0/5 Partial bundle follow upsDerrick Stolee via GitGitGadget, Mar 22, 2022
  2. 2/5 pack-objects: move revs out of get_object_list()Derrick Stolee via GitGitGadget, Mar 22, 2022
  3. 1/5 list-objects-filter: remove CL_ARG__FILTERDerrick Stolee via GitGitGadget, Mar 22, 2022
  4. 3/5 pack-objects: parse --filter directly into revs.filterDerrick Stolee via GitGitGadget, Mar 22, 2022
  5. Ramsay JonesMar 22, 2022
  6. Derrick StoleeMar 23, 2022
  7. Ævar Arnfjörð BjarmasonMar 22, 2022
  8. 5/5 bundle: output hash information in 'verify'Derrick Stolee via GitGitGadget, Mar 22, 2022
  9. 4/5 bundle: move capabilities to end of 'verify'Derrick Stolee via GitGitGadget, Mar 22, 2022
  10. Bagas SanjayaMar 23, 2022
  11. Derrick StoleeMar 23, 2022
  12. Junio C HamanoMar 23, 2022
  13. pack-objects: lazily set up "struct rev_info", don't leakÆvar Arnfjörð Bjarmason, Mar 25, 2022
  14. Derrick StoleeMar 25, 2022
  15. Ævar Arnfjörð BjarmasonMar 25, 2022
  16. Derrick StoleeMar 25, 2022
  17. Ævar Arnfjörð BjarmasonMar 25, 2022
  18. Derrick StoleeMar 25, 2022
  19. Ævar Arnfjörð BjarmasonMar 26, 2022
  20. Derrick StoleeMar 28, 2022
  21. Junio C HamanoMar 25, 2022
  22. Ævar Arnfjörð BjarmasonMar 26, 2022
  23. pack-objects: lazily set up "struct rev_info", don't leakÆvar Arnfjörð Bjarmason, Mar 28, 2022
  24. Derrick StoleeMar 28, 2022
  25. Junio C HamanoMar 28, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.