]> git.ipfire.org Git - thirdparty/git.git/commit - list-objects-filter-options.h
pack-objects: lazily set up "struct rev_info", don't leak
authorÆvar Arnfjörð Bjarmason <avarab@gmail.com>
Mon, 28 Mar 2022 15:43:18 +0000 (17:43 +0200)
committerJunio C Hamano <gitster@pobox.com>
Mon, 28 Mar 2022 16:57:21 +0000 (09:57 -0700)
commit5cb28270a1ff94a0a23e67b479bbbec3bc993518
treee5603fd2a923fff910bbe0f36cd33954c8a5c04e
parent8ba221e2453658f8843176cc00fc5cfe36e07b41
pack-objects: lazily set up "struct rev_info", don't leak

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.

An earlier version of this commit[3] went behind
opt_parse_list_objects_filter()'s back by faking up a "struct option"
before calling it. Let's avoid that and instead create a blessed API
for this pattern.

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

While we're at it add parentheses around the arguments to the OPT_*
macros in in list-objects-filter-options.h, as we need to change those
lines anyway. It doesn't matter in this case, but is good general
practice.

1. https://lore.kernel.org/git/619b757d98465dbc4995bdc11a5282fbfcbd3daa.1647970119.git.gitgitgadget@gmail.com
2. https://lore.kernel.org/git/97de926904988b89b5663bd4c59c011a1723a8f5.1647970119.git.gitgitgadget@gmail.com
3. https://lore.kernel.org/git/patch-1.1-193534b0f07-20220325T121715Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/pack-objects.c
list-objects-filter-options.c
list-objects-filter-options.h