On 3/10/2022 8:11 AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Mar 09 2022, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>>
>> In builtin/pack-objects.c, we use a 'filter_options' global to populate
>> the --filter=<X> argument. The previous change created a pointer to a
>> filter option in 'struct rev_info', so we can use that pointer here as a
>> start to simplifying some usage of object filters.
>>
>> Signed-off-by: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>> ---
>> builtin/pack-objects.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index ba2006f2212..e5b7d015d7d 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -3651,7 +3651,7 @@ static int pack_options_allow_reuse(void)
>>
>> static int get_object_list_from_bitmap(struct rev_info *revs)
>> {
>> - if (!(bitmap_git = prepare_bitmap_walk(revs, &filter_options, 0)))
>> + if (!(bitmap_git = prepare_bitmap_walk(revs, &revs->filter, 0)))
>> return -1;
>>
>> if (pack_options_allow_reuse() &&
>> @@ -3727,6 +3727,7 @@ static void get_object_list(int ac, const char **av)
>> repo_init_revisions(the_repository, &revs, NULL);
>> save_commit_buffer = 0;
>> setup_revisions(ac, av, &revs, &s_r_opt);
>> + list_objects_filter_copy(&revs.filter, &filter_options);
>>
>> /* make sure shallows are read */
>> is_repository_shallow(the_repository);
>> @@ -3777,7 +3778,7 @@ static void get_object_list(int ac, const char **av)
>>
>> if (!fn_show_object)
>> fn_show_object = show_object;
>> - traverse_commit_list_filtered(&filter_options, &revs,
>> + traverse_commit_list_filtered(&revs.filter, &revs,
>> show_commit, fn_show_object, NULL,
>> NULL);
>
> Re your
> https://lore.kernel.org/git/77c8ef4b-5dce-401b-e703-cfa32e18c853@xxxxxxxxxx/
> I was looking at how to handle the interaction between this and my
> revisions_release() series.
>
> This adds a new memory leak, which can be seen with:
>
> make SANITIZE=leak
> (cd t && ./t5532-fetch-proxy.sh -vixd)
>
> I.e. this part is new:
>
> remote: Direct leak of 1 byte(s) in 1 object(s) allocated from:
> remote: #0 0x4552f8 in __interceptor_malloc (git+0x4552f8)
> remote: #1 0x75a089 in do_xmalloc wrapper.c:41:8
> remote: #2 0x75a046 in xmalloc wrapper.c:62:9
> remote: #3 0x62c692 in list_objects_filter_copy list-objects-filter-options.c:433:2
> remote: #4 0x4f70bf in get_object_list builtin/pack-objects.c:3730:2
> remote: #5 0x4f5e0e in cmd_pack_objects builtin/pack-objects.c:4157:3
> remote: #6 0x4592ba in run_builtin git.c:465:11
> remote: #7 0x457d71 in handle_builtin git.c:718:3
> remote: #8 0x458ca5 in run_argv git.c:785:4
> remote: #9 0x457b30 in cmd_main git.c:916:19
> remote: #10 0x563179 in main common-main.c:56:11
> remote: #11 0x7fddd2da67ec in __libc_start_main csu/../csu/libc-start.c:332:16
> remote: #12 0x4300e9 in _start (git+0x4300e9)
>
> Of course it's not "new" in the sense that we in effect leaked this
> before, but it was still reachable, you're just changing it so that
> instead of being stored in the static "filter_options" variable in
> pack-objects.c we're storing it in "struct rev_info", which has a
> different lifetime.
True, and 'struct rev_info' is not being release in any way, either,
right?
> I think instead of me rebasing my series on top of yours and tangling
> the two up a better option is to just add a change to this, so after
> list_objects_filter_copy() do:
>
> UNLEAK(revs.filter);
>
> Or, alternatively adding this to the end of the function (in which case
> Junio will need to deal with a minor textual conflict):
>
> list_objects_filter_release(&revs.filter);
>
> Both of those make my series merged with "seen" (which has this change)
> pass with SANITIZE=leak + GIT_TEST_PASSING_SANITIZE_LEAK=true again.
>
> You could do the same in your later change adding
> list_objects_filter_copy() to verify_bundle(), that one also adds a new
> leak, but happens not to cause test failures since the bundle.c code
> isn't otherwise marked as passing with SANITIZE=leak, it fails in
> various other ways.
>
> Obviously we should do something about the actual leak eventually, but
> that can be done in some follow-up work to finish up the missing bits of
> release_revisions(), i.e. adding list_objects_filter_release() etc. to
> release_revisions().
I understand that you like to "show your work" by marking tests as
safe for leak-check by making the smallest changes possible, but your
series has a lot of small patches that do nothing but add a free() or
release_*() call instead of implementing the "right" release_revisions()
from the start.
> So I think just adding UNLEAK() here (and optionally, also to the
> bundle.c code) is the least invasive thing, if you & Junio are OK with
> that approach.
Could you send a patch that does just that, so we are sure to cover
the warnings you are seeing in your tests?
Thanks,
-Stolee
