git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 3/3] Revert "pack-objects: lazily set up "struct rev_info", don't leak"

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 28, 2022, 12:24 UTC
Message-ID
<221128.86zgcbl0pe.gmgdl@evledraar.gmail.com>
In-Reply-To
<221128.864jujmhgp.gmgdl@evledraar.gmail.com>
On Mon, Nov 28 2022, Ævar Arnfjörð Bjarmason wrote:
René:
Show 91 quoted lines
> On Mon, Nov 28 2022, René Scharfe wrote:
>
>> Am 28.11.2022 um 11:03 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.
>>>>
>>>> 5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak,
>>>> 2022-03-28) avoided leaking rev_info allocations in many cases by
>>>> calling repo_init_revisions() only when the .filter member was actually
>>>> needed, but then still leaking it.  That was fixed later by 2108fe4a19
>>>> (revisions API users: add straightforward release_revisions(),
>>>> 2022-04-13), making the reverted commit unnecessary.
>>>
>>> Hmph, with this merged, 'seen' breaks linux-leaks job in a strange
>>> way.
>>>
>>> https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917
>>>
>>> Does anybody want to help looking into it?
>
> [I see we crossed E-Mails]:
> https://lore.kernel.org/git/221128.868rjvmi3l.gmgdl@evledraar.gmail.com/
>
>> The patch exposes that release_revisions() leaks the diffopt allocations
>> as we're yet to address the TODO added by 54c8a7c379 (revisions API: add
>> a TODO for diff_free(&revs->diffopt), 2022-04-14).
>
> That's correct, and we have that leak in various places in our codebase,
> but per the above side-thread I think this is primarily exposing that
> we're setting up the "struct rev_info" with your change when we don't
> need to. Why can't we just skip it?
>
> Yeah, if we do set it up we'll run into an outstanding leak, and that
> should also be fixed (I have some local patches...), but the other cases
> I know of where we'll leak that data is where we're actually using the
> "struct rev_info".
>
> I haven't tried tearing your change apart to poke at it myself, and
> maybe there's some really good reason for why you can't separate getting
> rid of the J.5.7 dependency and removing the lazy-init.
>
>> The patch below plugs it locally.
>>
>> --- >8 ---
>> Subject: [PATCH 4/3] fixup! revision: free diffopt in release_revisions()
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  builtin/pack-objects.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index 3e74fbb0cd..a47a3f0fba 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -4462,6 +4462,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>>  	} else {
>>  		get_object_list(&revs, rp.nr, rp.v);
>>  	}
>> +	diff_free(&revs.diffopt);
>>  	release_revisions(&revs);
>>  	cleanup_preferred_base();
>>  	if (include_tag && nr_result)
>
> So, the main motivation for the change was paranoia that a compiler or
> platform might show up without J.5.7 support and that would bite us, but
> we're now adding a double-free-in-waiting?
>
> I think we're both a bit paranoid, but clearly have different
> paranoia-priorities :)
>
> If we do end up with some hack like this instead of fixing the
> underlying problem I'd much prefer that such a hack just be an UNLEAK()
> here.
>
> I.e. we have a destructor for "revs.*" already, let's not bypass it and
> start freeing things from under it, which will result in a double-free
> if we forget this callsite once the TODO in 54c8a7c379 is addressed.
>
> As you'd see if you made release_revisions() simply call
> diff_free(&revs.diffopt) doing so would reveal some really gnarly edge
> cases.
>
> I haven't dug into this one, but offhand I'm not confident in saying
> that this isn't exposing us to some aspect of that gnarlyness (maybe
> not, it's been a while since I looked).
>
> (IIRC some of the most gnarly edge cases will only show up as CI
> failures on Windows, to do with the ordering of when we'll fclose()
> files hanging off that "diffopt").

This squashed into 3/3 seems to me to be a proper fix to a change that wants to refactor the code for non-J.5.7 compatibility. I.e. this just does the data<->fp casting part of the change, without refactoring the "lazy init".

But I think you should check this a bit more carefully. Your 3/3 says that your change "mak[es] the reverted commit unnecessary", but as I noted if you'd run the command that commit shows, you'd have seen you're re-introducing the leak it fixed. So I wonder what else has been missed here.

I vaguely recall that one reason I ended up with that J.5.7 dependency was because there was an objection to mocking up the "struct option" as I'm doing here. I.e. here we assume that the opt_parse_list_objects_filter() is only ever going to care about the "value" member.

I think that's probably fine, but I may be misrecalling, or missing some crucial detail. I'll leave digging that up & convincing us that it's fine to the person pushing for refactoring all of this :)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3e74fbb0cd5..faf210bfe8c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4149,6 +4149,27 @@ static int option_parse_cruft_expiration(const struct option *opt,
 	return 0;
 }
 
+struct po_filter_data {
+	unsigned have_revs:1;
+	struct rev_info revs;
+};
+
+static int opt_parse_list_objects_filter_init(const struct option *opt,
+					      const char *arg, int unset)
+{
+	struct po_filter_data *data = opt->value;
+	struct rev_info *revs = &data->revs;
+	const struct option opt_rev = {
+		.value = (void *)&revs->filter,
+	};
+
+	if (!data->have_revs)
+		repo_init_revisions(the_repository, revs, NULL);
+	data->have_revs = 1;
+
+	return opt_parse_list_objects_filter(&opt_rev, arg, unset);
+}
+
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
@@ -4159,7 +4180,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	int rev_list_index = 0;
 	int stdin_packs = 0;
 	struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
-	struct rev_info revs;
+	struct po_filter_data pfd = { .have_revs = 0 };
 
 	struct option pack_objects_options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
@@ -4250,7 +4271,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			      &write_bitmap_index,
 			      N_("write a bitmap index if possible"),
 			      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),
-		OPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),
+		OPT_CALLBACK(0, "filter", &pfd, N_("args"), N_("object filtering"),
+			     opt_parse_list_objects_filter_init),
 		OPT_CALLBACK_F(0, "missing", NULL, N_("action"),
 		  N_("handling for missing objects"), PARSE_OPT_NONEG,
 		  option_parse_missing_action),
@@ -4269,8 +4291,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 
 	read_replace_refs = 0;
 
-	repo_init_revisions(the_repository, &revs, NULL);
-
 	sparse = git_env_bool("GIT_TEST_PACK_SPARSE", -1);
 	if (the_repository->gitdir) {
 		prepare_repo_settings(the_repository);
@@ -4372,7 +4392,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!rev_list_all || !rev_list_reflog || !rev_list_index)
 		unpack_unreachable_expiration = 0;
 
-	if (revs.filter.choice) {
+	if (pfd.have_revs && pfd.revs.filter.choice) {
 		if (!pack_to_stdout)
 			die(_("cannot use --filter without --stdout"));
 		if (stdin_packs)
@@ -4459,10 +4479,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		read_cruft_objects();
 	} else if (!use_internal_rev_list) {
 		read_object_list_from_stdin();
+	} else if (pfd.have_revs) {
+		get_object_list(&pfd.revs, rp.nr, rp.v);
+		release_revisions(&pfd.revs);
 	} else {
+		struct rev_info revs;
+
+		repo_init_revisions(the_repository, &revs, NULL);
 		get_object_list(&revs, rp.nr, rp.v);
+		release_revisions(&revs);
 	}
-	release_revisions(&revs);
 	cleanup_preferred_base();
 	if (include_tag && nr_result)
 		for_each_tag_ref(add_ref_tag, NULL);
Previous: Ævar Arnfjörð BjarmasonNext: René Scharfe
Message 22 of 52 in “pack-objects: fix and simplify --filter handling”
  1. 0/3 pack-objects: fix and simplify --filter handlingRené Scharfe, Nov 12, 2022
  2. 1/3 pack-objects: fix handling of multiple --filter optionsRené Scharfe, Nov 12, 2022
  3. Ævar Arnfjörð BjarmasonNov 12, 2022
  4. René ScharfeNov 13, 2022
  5. Jeff KingNov 12, 2022
  6. Taylor BlauNov 13, 2022
  7. Jeff KingNov 13, 2022
  8. René ScharfeNov 13, 2022
  9. 2/3 pack-object: simplify --filter handlingRené Scharfe, Nov 12, 2022
  10. Ævar Arnfjörð BjarmasonNov 12, 2022
  11. Jeff KingNov 12, 2022
  12. Jeff KingNov 13, 2022
  13. René ScharfeNov 13, 2022
  14. 3/3 list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()René Scharfe, Nov 12, 2022
  15. 0/3 pack-objects: fix and simplify --filter handlingRené Scharfe, Nov 20, 2022
  16. 2/3 t5317: demonstrate failure to handle multiple --filter optionsRené Scharfe, Nov 20, 2022
  17. 1/3 t5317: stop losing return codes of git ls-filesRené Scharfe, Nov 20, 2022
  18. 3/3 Revert "pack-objects: lazily set up "struct rev_info", don't leak"René Scharfe, Nov 20, 2022
  19. Junio C HamanoNov 28, 2022
  20. René ScharfeNov 28, 2022
  21. Ævar Arnfjörð BjarmasonNov 28, 2022
  22. Ævar Arnfjörð BjarmasonNov 28, 2022
  23. René ScharfeNov 28, 2022
  24. Ævar Arnfjörð BjarmasonNov 28, 2022
  25. René ScharfeNov 28, 2022
  26. Ævar Arnfjörð BjarmasonNov 28, 2022
  27. René ScharfeNov 28, 2022
  28. René ScharfeNov 28, 2022
  29. Ævar Arnfjörð BjarmasonNov 28, 2022
  30. René ScharfeNov 28, 2022
  31. Jeff KingNov 29, 2022
  32. Junio C HamanoNov 29, 2022
  33. Ævar Arnfjörð BjarmasonNov 29, 2022
  34. Ævar Arnfjörð BjarmasonNov 29, 2022
  35. René ScharfeNov 29, 2022
  36. Ævar Arnfjörð BjarmasonNov 28, 2022
  37. Ævar Arnfjörð BjarmasonNov 28, 2022
  38. t5314: check exit code of "rev-parse"Ævar Arnfjörð Bjarmason, Nov 28, 2022
  39. René ScharfeNov 28, 2022
  40. t5314: check exit code of "git"Ævar Arnfjörð Bjarmason, Nov 28, 2022
  41. Jeff KingNov 22, 2022
  42. 0/5 pack-objects: fix and simplify --filter handlingRené Scharfe, Nov 29, 2022
  43. 1/5 t5317: stop losing return codes of git ls-filesRené Scharfe, Nov 29, 2022
  44. 2/5 t5317: demonstrate failure to handle multiple --filter optionsRené Scharfe, Nov 29, 2022
  45. 3/5 pack-objects: fix handling of multiple --filter optionsRené Scharfe, Nov 29, 2022
  46. Junio C HamanoNov 30, 2022
  47. René ScharfeNov 30, 2022
  48. 4/5 pack-objects: simplify --filter handlingRené Scharfe, Nov 29, 2022
  49. Ævar Arnfjörð BjarmasonNov 29, 2022
  50. René ScharfeNov 30, 2022
  51. 5/5 list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()René Scharfe, Nov 29, 2022
  52. Junio C HamanoNov 30, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.