[v3,07/12] list-objects: handle NULL function pointers

Commit Message

Ævar Arnfjörð Bjarmason March 8, 2022, 2:39 p.m. UTC

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <[e-mail]>

If a caller to traverse_commit_list() specifies the options for the
--objects flag but does not specify a show_object function pointer, the
result is a segfault. This is currently visible by running 'git bundle
create --objects HEAD'.

We could fix this problem by supplying a no-op callback in
builtin/bundle.c, but that only solves the problem for one builtin,
leaving this segfault open for other callers.

Replace all callers of the show_commit and show_object function pointers
in list-objects.c to be local methods show_commit() and show_object()
which check that the given contex has non-NULL functions before passing
the necessary data. One extra benefit is that it reduces duplication
due to passing ctx->show_data to every caller.

Test that this segfault no longer occurs for 'git bundle'.

Co-authored-by: Derrick Stolee <[e-mail]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 bundle.c               |  2 ++
 list-objects.c         | 27 ++++++++++++++++++++++-----
 t/t6020-bundle-misc.sh | 12 ++++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

Comments

Junio C Hamano March 8, 2022, 5:26 p.m. UTC | #1

"Ævar Arnfjörð Bjarmason via GitGitGadget"  <[e-mail]>
writes:

> Replace all callers of the show_commit and show_object function pointers
> in list-objects.c to be local methods show_commit() and show_object()

"to be local methods" -> "to call helper functions"

> which check that the given contex has non-NULL functions before passing

"contex" -> "context"

> the necessary data. One extra benefit is that it reduces duplication
> due to passing ctx->show_data to every caller.

> -		ctx->show_object(obj, path->buf, ctx->show_data);
> +		show_object(ctx, obj, path->buf);

I guess this is the "reduced duplication" refers to.  The helper
does make it easier to follow and reason about: "show the given
object at the path in this context" is what it asks.

> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
> index b13e8a52a93..6522401617d 100755
> --- a/t/t6020-bundle-misc.sh
> +++ b/t/t6020-bundle-misc.sh
> @@ -475,4 +475,16 @@ test_expect_success 'clone from bundle' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'unfiltered bundle with --objects' '
> +	git bundle create all-objects.bdl \
> +		--all --objects &&
> +	git bundle create all.bdl \
> +		--all &&
> +
> +	# Compare the headers of these files.
> +	head -11 all.bdl >expect &&
> +	head -11 all-objects.bdl >actual &&

"head -n 11" but more importantly, why eleven and not ten or twelve?
Is that a number this code can automatically learn from the given
.bdl file?

Ævar Arnfjörð Bjarmason March 9, 2022, 1:40 p.m. UTC | #2

On Tue, Mar 08 2022, Junio C Hamano wrote:

> "Ævar Arnfjörð Bjarmason via GitGitGadget"  <[e-mail]>
> writes:
>
>> Replace all callers of the show_commit and show_object function pointers
>> in list-objects.c to be local methods show_commit() and show_object()
>
> "to be local methods" -> "to call helper functions"
>
>> which check that the given contex has non-NULL functions before passing
>
> "contex" -> "context"
>
>> the necessary data. One extra benefit is that it reduces duplication
>> due to passing ctx->show_data to every caller.
>
>> -		ctx->show_object(obj, path->buf, ctx->show_data);
>> +		show_object(ctx, obj, path->buf);
>
> I guess this is the "reduced duplication" refers to.  The helper
> does make it easier to follow and reason about: "show the given
> object at the path in this context" is what it asks.
>
>> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
>> index b13e8a52a93..6522401617d 100755
>> --- a/t/t6020-bundle-misc.sh
>> +++ b/t/t6020-bundle-misc.sh
>> @@ -475,4 +475,16 @@ test_expect_success 'clone from bundle' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success 'unfiltered bundle with --objects' '
>> +	git bundle create all-objects.bdl \
>> +		--all --objects &&
>> +	git bundle create all.bdl \
>> +		--all &&
>> +
>> +	# Compare the headers of these files.
>> +	head -11 all.bdl >expect &&
>> +	head -11 all-objects.bdl >actual &&
>
> "head -n 11" but more importantly, why eleven and not ten or twelve?
> Is that a number this code can automatically learn from the given
> .bdl file?

I suspect what's wanted here is "print all stuff before the "\n\n"
header/PACK delimiter, which is better done with "sed" like this:

	sed -n -e '/^$/q' -e 'p'

Derrick Stolee March 9, 2022, 2:16 p.m. UTC | #3

On 3/9/2022 8:40 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 08 2022, Junio C Hamano wrote:
> 
>> "Ævar Arnfjörð Bjarmason via GitGitGadget"  <[e-mail]>
>> writes:
>>
>>> Replace all callers of the show_commit and show_object function pointers
>>> in list-objects.c to be local methods show_commit() and show_object()
>>
>> "to be local methods" -> "to call helper functions"
>>
>>> which check that the given contex has non-NULL functions before passing
>>
>> "contex" -> "context"
>>
>>> the necessary data. One extra benefit is that it reduces duplication
>>> due to passing ctx->show_data to every caller.
>>
>>> -		ctx->show_object(obj, path->buf, ctx->show_data);
>>> +		show_object(ctx, obj, path->buf);
>>
>> I guess this is the "reduced duplication" refers to.  The helper
>> does make it easier to follow and reason about: "show the given
>> object at the path in this context" is what it asks.
>>
>>> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
>>> index b13e8a52a93..6522401617d 100755
>>> --- a/t/t6020-bundle-misc.sh
>>> +++ b/t/t6020-bundle-misc.sh
>>> @@ -475,4 +475,16 @@ test_expect_success 'clone from bundle' '
>>>  	test_cmp expect actual
>>>  '
>>>  
>>> +test_expect_success 'unfiltered bundle with --objects' '
>>> +	git bundle create all-objects.bdl \
>>> +		--all --objects &&
>>> +	git bundle create all.bdl \
>>> +		--all &&
>>> +
>>> +	# Compare the headers of these files.
>>> +	head -11 all.bdl >expect &&
>>> +	head -11 all-objects.bdl >actual &&
>>
>> "head -n 11" but more importantly, why eleven and not ten or twelve?
>> Is that a number this code can automatically learn from the given
>> .bdl file?
> 
> I suspect what's wanted here is "print all stuff before the "\n\n"
> header/PACK delimiter, which is better done with "sed" like this:
> 
> 	sed -n -e '/^$/q' -e 'p'

Thanks for this tip. That is indeed the intention.

-Stolee

Junio C Hamano March 9, 2022, 6:32 p.m. UTC | #4

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> I suspect what's wanted here is "print all stuff before the "\n\n"
> header/PACK delimiter, which is better done with "sed" like this:
>
> 	sed -n -e '/^$/q' -e 'p'

I see.  Or just "sed -e '/^$/q'" would also be fine (i.e. "print
everything up to, including the first blank line") for comparison
purposes and may be simpler.

Thanks.

Patch

diff --git a/bundle.c b/bundle.c
index a0bb687b0f4..7ba60a573d7 100644
--- a/bundle.c
+++ b/bundle.c
@@ -544,6 +544,8 @@  int create_bundle(struct repository *r, const char *path,
 		die("revision walk setup failed");
 	bpi.fd = bundle_fd;
 	bpi.pending = &revs_copy.pending;
+
+	revs.blob_objects = revs.tree_objects = 0;
 	traverse_commit_list(&revs, write_bundle_prerequisites, NULL, &bpi);
 	object_array_remove_duplicates(&revs_copy.pending);
 
diff --git a/list-objects.c b/list-objects.c
index 9422625b39e..0af0bef1dbc 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -21,6 +21,23 @@  struct traversal_context {
 	struct filter *filter;
 };
 
+static void show_commit(struct traversal_context *ctx,
+			struct commit *commit)
+{
+	if (!ctx->show_commit)
+		return;
+	ctx->show_commit(commit, ctx->show_data);
+}
+
+static void show_object(struct traversal_context *ctx,
+			struct object *object,
+			const char *name)
+{
+	if (!ctx->show_object)
+		return;
+	ctx->show_object(object, name, ctx->show_data);
+}
+
 static void process_blob(struct traversal_context *ctx,
 			 struct blob *blob,
 			 struct strbuf *path,
@@ -60,7 +77,7 @@  static void process_blob(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		obj->flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(obj, path->buf, ctx->show_data);
+		show_object(ctx, obj, path->buf);
 	strbuf_setlen(path, pathlen);
 }
 
@@ -194,7 +211,7 @@  static void process_tree(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		obj->flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(obj, base->buf, ctx->show_data);
+		show_object(ctx, obj, base->buf);
 	if (base->len)
 		strbuf_addch(base, '/');
 
@@ -210,7 +227,7 @@  static void process_tree(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		obj->flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(obj, base->buf, ctx->show_data);
+		show_object(ctx, obj, base->buf);
 
 	strbuf_setlen(base, baselen);
 	free_tree_buffer(tree);
@@ -228,7 +245,7 @@  static void process_tag(struct traversal_context *ctx,
 	if (r & LOFR_MARK_SEEN)
 		tag->object.flags |= SEEN;
 	if (r & LOFR_DO_SHOW)
-		ctx->show_object(&tag->object, name, ctx->show_data);
+		show_object(ctx, &tag->object, name);
 }
 
 static void mark_edge_parents_uninteresting(struct commit *commit,
@@ -402,7 +419,7 @@  static void do_traverse(struct traversal_context *ctx)
 		if (r & LOFR_MARK_SEEN)
 			commit->object.flags |= SEEN;
 		if (r & LOFR_DO_SHOW)
-			ctx->show_commit(commit, ctx->show_data);
+			show_commit(ctx, commit);
 
 		if (ctx->revs->tree_blobs_in_commit_order)
 			/*
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index b13e8a52a93..6522401617d 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -475,4 +475,16 @@  test_expect_success 'clone from bundle' '
 	test_cmp expect actual
 '
 
+test_expect_success 'unfiltered bundle with --objects' '
+	git bundle create all-objects.bdl \
+		--all --objects &&
+	git bundle create all.bdl \
+		--all &&
+
+	# Compare the headers of these files.
+	head -11 all.bdl >expect &&
+	head -11 all-objects.bdl >actual &&
+	test_cmp expect actual
+'
+
 test_done