Patrick Steinhardt <ps@xxxxxx> writes:
> This one is a little bit more curious. In t6112, we have a test that
> exercises the `git rev-list --filter` option with invalid filters. We
> execute git-rev-list(1) via `test_must_fail`, which means that we check
> for leaks even though Git exits with an error code. This causes the
> following leak:
>
> Direct leak of 27 byte(s) in 1 object(s) allocated from:
> #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o
> #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8
> #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2
> #3 0x5555558b7550 in strbuf_add strbuf.c:311:2
> #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2
> #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3
> #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3
> #7 0x555555884e20 in setup_revisions revision.c:3014:11
> #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9
> #9 0x5555555ec5e3 in run_builtin git.c:483:11
> #10 0x5555555eb1e4 in handle_builtin git.c:749:13
> #11 0x5555555ec001 in run_argv git.c:819:4
> #12 0x5555555eaf94 in cmd_main git.c:954:19
> #13 0x5555556fd569 in main common-main.c:64:11
> #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d)
> #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208)
> #16 0x5555555ad064 in _start (git+0x59064)
>
> This leak is valid, as we call `die()` and do not clean up the memory at
> all. But what's curious is that this is the only leak reported, because
> we don't clean up any other allocated memory, either, and I have no idea
> why the leak sanitizer treats this buffer specially.
>
> In any case, we can work around the leak by shuffling things around a
> bit. Instead of calling `gently_parse_list_objects_filter()` and dying
> after we have modified the filter spec, we simply do so beforehand. Like
> this we don't allocate the buffer in the error case, which makes the
> reported leak go away.
>
> It's not pretty, but it manages to make t6112 leak free.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> list-objects-filter-options.c | 17 +++++++----------
> t/t6112-rev-list-filters-objects.sh | 1 +
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 00611107d20..fa72e81e4ad 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -252,16 +252,14 @@ void parse_list_objects_filter(
> const char *arg)
> {
> struct strbuf errbuf = STRBUF_INIT;
> - int parse_error;
>
> if (!filter_options->filter_spec.buf)
> BUG("filter_options not properly initialized");
>
> if (!filter_options->choice) {
> + if (gently_parse_list_objects_filter(filter_options, arg, &errbuf))
> + die("%s", errbuf.buf);
> strbuf_addstr(&filter_options->filter_spec, arg);
> -
> - parse_error = gently_parse_list_objects_filter(
> - filter_options, arg, &errbuf);
> } else {
> struct list_objects_filter_options *sub;
>
> @@ -271,18 +269,17 @@ void parse_list_objects_filter(
> */
> transform_to_combine_type(filter_options);
>
> - strbuf_addch(&filter_options->filter_spec, '+');
> - filter_spec_append_urlencode(filter_options, arg);
> ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
> filter_options->sub_alloc);
> sub = &filter_options->sub[filter_options->sub_nr - 1];
>
> list_objects_filter_init(sub);
> - parse_error = gently_parse_list_objects_filter(sub, arg,
> - &errbuf);
> + if (gently_parse_list_objects_filter(sub, arg, &errbuf))
> + die("%s", errbuf.buf);
Do we actually have a test hitting this code path? I wanted to figure
out why `filter_options->sub` wasn't leaky (I assume that's what you're
talking about in your commit message), but I wasn't able to reproduce a
scenario where we should die here.
--
Toon
