git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: Taylor Blau <me@ttaylorr.com>, Toon Claes <toon@iotcl.com>,
	Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>,
	Justin Tobler <jltobler@gmail.com>
Subject: [PATCH v3 22/22] list-objects-filter-options: work around reported leak on error
Date: Tue, 5 Nov 2024 07:17:54 +0100	[thread overview]
Message-ID: <d9e122bb5db965cdc3d86f4ba869cd51a3aca800.1730786196.git.ps@pks.im> (raw)
In-Reply-To: <cover.1730786195.git.ps@pks.im>

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);
+
+		strbuf_addch(&filter_options->filter_spec, '+');
+		filter_spec_append_urlencode(filter_options, arg);
 	}
-	if (parse_error)
-		die("%s", errbuf.buf);
 }
 
 int opt_parse_list_objects_filter(const struct option *opt,
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 0387f35a326..71e38491fa8 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -5,6 +5,7 @@ test_description='git rev-list using object filtering'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the blob:none filter.
-- 
2.47.0.229.g8f8d6eee53.dirty


  parent reply	other threads:[~2024-11-05  6:18 UTC|newest]

Thread overview: 100+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-11  5:32 [PATCH 00/21] Memory leak fixes (pt.9) Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 01/21] builtin/ls-remote: plug leaking server options Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 02/21] t/helper: fix leaks in "reach" test tool Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 03/21] grep: fix leak in `grep_splice_or()` Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 04/21] builtin/grep: fix leak with `--max-count=0` Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 05/21] revision: fix leaking bloom filters Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 06/21] diff-lib: fix leaking diffopts in `do_diff_cache()` Patrick Steinhardt
2024-10-21  9:46   ` Kristoffer Haugsbakk
2024-10-11  5:32 ` [PATCH 07/21] pretty: clear signature check Patrick Steinhardt
2024-10-18 12:02   ` Toon Claes
2024-10-21  8:44     ` Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 08/21] upload-pack: fix leaking URI protocols Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 09/21] builtin/commit: fix leaking change data contents Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 10/21] trailer: fix leaking trailer values Patrick Steinhardt
2024-10-18 12:03   ` Toon Claes
2024-10-21  8:44     ` Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 11/21] builtin/commit: fix leaking cleanup config Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 12/21] transport-helper: fix leaking import/export marks Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 13/21] builtin/tag: fix leaking key ID on failure to sign Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 14/21] combine-diff: fix leaking lost lines Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 15/21] dir: release untracked cache data Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 16/21] sparse-index: correctly free EWAH contents Patrick Steinhardt
2024-10-11  5:32 ` [PATCH 17/21] t/helper: stop re-initialization of `the_repository` Patrick Steinhardt
2024-10-11  5:33 ` [PATCH 18/21] t/helper: fix leaking buffer in "dump-untracked-cache" Patrick Steinhardt
2024-10-11  5:33 ` [PATCH 19/21] dir: fix leak when parsing "status.showUntrackedFiles" Patrick Steinhardt
2024-10-11  5:33 ` [PATCH 20/21] builtin/merge: release outbut buffer after performing merge Patrick Steinhardt
2024-10-18 12:03   ` Toon Claes
2024-10-21  8:45     ` Patrick Steinhardt
2024-10-11  5:33 ` [PATCH 21/21] list-objects-filter-options: work around reported leak on error Patrick Steinhardt
2024-10-18 12:04   ` Toon Claes
2024-10-21  8:45     ` Patrick Steinhardt
2024-10-18 21:30 ` [PATCH 00/21] Memory leak fixes (pt.9) Taylor Blau
2024-10-21  8:45   ` Patrick Steinhardt
2024-10-21  9:27 ` [PATCH v2 00/22] " Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 01/22] builtin/ls-remote: plug leaking server options Patrick Steinhardt
2024-11-04 22:10     ` Justin Tobler
2024-11-04 22:18       ` Kristoffer Haugsbakk
2024-10-21  9:28   ` [PATCH v2 02/22] t/helper: fix leaks in "reach" test tool Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 03/22] grep: fix leak in `grep_splice_or()` Patrick Steinhardt
2024-10-21  9:42     ` Kristoffer Haugsbakk
2024-10-21  9:28   ` [PATCH v2 04/22] builtin/grep: fix leak with `--max-count=0` Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 05/22] revision: fix leaking bloom filters Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 06/22] diff-lib: fix leaking diffopts in `do_diff_cache()` Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 07/22] pretty: clear signature check Patrick Steinhardt
2024-11-04 22:15     ` Justin Tobler
2024-10-21  9:28   ` [PATCH v2 08/22] upload-pack: fix leaking URI protocols Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 09/22] builtin/commit: fix leaking change data contents Patrick Steinhardt
2024-11-04 22:21     ` Justin Tobler
2024-10-21  9:28   ` [PATCH v2 10/22] trailer: fix leaking trailer values Patrick Steinhardt
2024-11-04 22:25     ` Justin Tobler
2024-11-05  5:54       ` Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 11/22] trailer: fix leaking strbufs when formatting trailers Patrick Steinhardt
2024-10-21 20:58     ` Taylor Blau
2024-11-04 22:31     ` Justin Tobler
2024-11-05  5:54       ` Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 12/22] builtin/commit: fix leaking cleanup config Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 13/22] transport-helper: fix leaking import/export marks Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 14/22] builtin/tag: fix leaking key ID on failure to sign Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 15/22] combine-diff: fix leaking lost lines Patrick Steinhardt
2024-11-04 22:43     ` Justin Tobler
2024-11-05  5:55       ` Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 16/22] dir: release untracked cache data Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 17/22] sparse-index: correctly free EWAH contents Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 18/22] t/helper: stop re-initialization of `the_repository` Patrick Steinhardt
2024-10-21  9:28   ` [PATCH v2 19/22] t/helper: fix leaking buffer in "dump-untracked-cache" Patrick Steinhardt
2024-10-21  9:29   ` [PATCH v2 20/22] dir: fix leak when parsing "status.showUntrackedFiles" Patrick Steinhardt
2024-10-21  9:29   ` [PATCH v2 21/22] builtin/merge: release outbut buffer after performing merge Patrick Steinhardt
2024-10-21  9:41     ` Kristoffer Haugsbakk
2024-10-21  9:29   ` [PATCH v2 22/22] list-objects-filter-options: work around reported leak on error Patrick Steinhardt
2024-10-21  9:54   ` [PATCH v2 00/22] Memory leak fixes (pt.9) Kristoffer Haugsbakk
2024-10-21 10:36     ` Patrick Steinhardt
2024-10-25  7:49       ` Toon Claes
2024-11-04 22:46   ` Justin Tobler
2024-11-05  5:55     ` Patrick Steinhardt
2024-11-05  6:16 ` [PATCH v3 " Patrick Steinhardt
2024-11-05  6:16   ` [PATCH v3 01/22] builtin/ls-remote: plug leaking server options Patrick Steinhardt
2024-11-05  6:16   ` [PATCH v3 02/22] t/helper: fix leaks in "reach" test tool Patrick Steinhardt
2024-11-05  6:16   ` [PATCH v3 03/22] grep: fix leak in `grep_splice_or()` Patrick Steinhardt
2024-11-05  6:16   ` [PATCH v3 04/22] builtin/grep: fix leak with `--max-count=0` Patrick Steinhardt
2024-11-05  6:16   ` [PATCH v3 05/22] revision: fix leaking bloom filters Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 06/22] diff-lib: fix leaking diffopts in `do_diff_cache()` Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 07/22] pretty: clear signature check Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 08/22] upload-pack: fix leaking URI protocols Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 09/22] builtin/commit: fix leaking change data contents Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 10/22] trailer: fix leaking trailer values Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 11/22] trailer: fix leaking strbufs when formatting trailers Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 12/22] builtin/commit: fix leaking cleanup config Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 13/22] transport-helper: fix leaking import/export marks Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 14/22] builtin/tag: fix leaking key ID on failure to sign Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 15/22] combine-diff: fix leaking lost lines Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 16/22] dir: release untracked cache data Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 17/22] sparse-index: correctly free EWAH contents Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 18/22] t/helper: stop re-initialization of `the_repository` Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 19/22] t/helper: fix leaking buffer in "dump-untracked-cache" Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 20/22] dir: fix leak when parsing "status.showUntrackedFiles" Patrick Steinhardt
2024-11-05  6:17   ` [PATCH v3 21/22] builtin/merge: release output buffer after performing merge Patrick Steinhardt
2024-11-05  6:17   ` Patrick Steinhardt [this message]
2024-11-05  6:51   ` [PATCH v3 00/22] Memory leak fixes (pt.9) Junio C Hamano
2024-11-05  6:52     ` Patrick Steinhardt
2024-11-05 15:27       ` Justin Tobler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9e122bb5db965cdc3d86f4ba869cd51a3aca800.1730786196.git.ps@pks.im \
    --to=ps@pks.im \
    --cc=git@vger.kernel.org \
    --cc=jltobler@gmail.com \
    --cc=kristofferhaugsbakk@fastmail.com \
    --cc=me@ttaylorr.com \
    --cc=toon@iotcl.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).