Re: [PATCH 21/21] list-objects-filter-options: work around reported leak on error

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> 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@pks.im>
> ---
>  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