[PATCH] bundle: do not free uninitialized memory on bad arguments

[PATCH] bundle: do not free uninitialized memory on bad arguments

[Olof Fairbrook]

"git bundle create" gives up with an error when setup_revisions()
leaves an argument it does not understand, as in

  $ git bundle create private.bundle main --no-such-option
  error: unrecognized argument: --no-such-option

Since 11ee9a75e7 (bundle: plug leaks in `create_bundle()`,
2024-06-11), that error jumps to the cleanup at the end of
create_bundle(), which clears "revs_copy.pending".  But "revs_copy"
is only filled from "revs" further down, so the cleanup reads
uninitialized stack memory and may free a garbage pointer.  valgrind
reports "Conditional jump or move depends on uninitialised value(s)"
in object_array_clear() and in free(); whether the command crashes
depends on what happens to be on the stack.

Give “revs_copy” its value where it is declared, and add a test that
fails under valgrind without this fix.

Signed-off-by: Olof Fairbrook <olof.fairbrook@gmail.com>
---
 bundle.c                | 2 +-
 t/t5607-clone-bundle.sh | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/bundle.c b/bundle.c
index f55a521b2a..aad5f251ca 100644
--- a/bundle.c
+++ b/bundle.c
@@ -492,7 +492,7 @@ int create_bundle(struct repository *r, const char *path,
 	int bundle_fd = -1;
 	int bundle_to_stdout;
 	int ref_count = 0;
-	struct rev_info revs, revs_copy;
+	struct rev_info revs, revs_copy = { 0 };
 	int min_version = 2;
 	struct bundle_prerequisites_info bpi;
 	int ret;
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index d709bea753..c39db5a955 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -127,6 +127,13 @@ test_expect_success 'failed bundle creation does not leave cruft' '
 	test_path_is_missing fail.bundle.lock
 '
 
+test_expect_success 'an unknown argument after the revisions is an error' '
+	test_must_fail git bundle create unknown.bundle main --no-such-option \
+		2>err &&
+	test_grep "unrecognized argument: --no-such-option" err &&
+	test_path_is_missing unknown.bundle
+'
+
 test_expect_success 'fetch SHA-1 from bundle' '
 	test_create_repo foo &&
 	test_commit -C foo x &&

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
2.43.0

Re: [PATCH] bundle: do not free uninitialized memory on bad arguments

[Patrick Steinhardt]

On Sun, Oct 18, 2026 at 04:21:29PM +0200, Olof Fairbrook wrote:
> "git bundle create" gives up with an error when setup_revisions()
> leaves an argument it does not understand, as in
> 
>   $ git bundle create private.bundle main --no-such-option
>   error: unrecognized argument: --no-such-option
> 
> Since 11ee9a75e7 (bundle: plug leaks in `create_bundle()`,
> 2024-06-11), that error jumps to the cleanup at the end of
> create_bundle(), which clears "revs_copy.pending".  But "revs_copy"
> is only filled from "revs" further down, so the cleanup reads
> uninitialized stack memory and may free a garbage pointer.  valgrind
> reports "Conditional jump or move depends on uninitialised value(s)"
> in object_array_clear() and in free(); whether the command crashes
> depends on what happens to be on the stack.

Nit: that last sentence is a bit on the verbose side, as it doesn't
really give any more useful information. I'd just drop it.

> diff --git a/bundle.c b/bundle.c
> index f55a521b2a..aad5f251ca 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -492,7 +492,7 @@ int create_bundle(struct repository *r, const char *path,
>  	int bundle_fd = -1;
>  	int bundle_to_stdout;
>  	int ref_count = 0;
> -	struct rev_info revs, revs_copy;
> +	struct rev_info revs, revs_copy = { 0 };
>  	int min_version = 2;
>  	struct bundle_prerequisites_info bpi;
>  	int ret;

Right. And for those wondering: we also release `revs` and `lock` on
exit, but we always call `setup_revisions(&revs)` before any `goto out`
and `lock` is initialized with `LOCK_INIT`. So both of those are fine.

> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> index d709bea753..c39db5a955 100755
> --- a/t/t5607-clone-bundle.sh
> +++ b/t/t5607-clone-bundle.sh
> @@ -127,6 +127,13 @@ test_expect_success 'failed bundle creation does not leave cruft' '
>  	test_path_is_missing fail.bundle.lock
>  '
>  
> +test_expect_success 'an unknown argument after the revisions is an error' '
> +	test_must_fail git bundle create unknown.bundle main --no-such-option \
> +		2>err &&

Wrapping only for the redirect is a bit funny, but oh, well.

> +	test_grep "unrecognized argument: --no-such-option" err &&
> +	test_path_is_missing unknown.bundle
> +'
> +
>  test_expect_success 'fetch SHA-1 from bundle' '
>  	test_create_repo foo &&
>  	test_commit -C foo x &&

I think overall this looks good enough already, and the nits I have
raised do not really need to be addressed.

Thanks!

Patrick

Re: [PATCH] bundle: do not free uninitialized memory on bad arguments

[Olof Fairbrook]

On Mon, 19 Oct 2026, Patrick Steinhardt wrote:

>> in object_array_clear() and in free(); whether the command crashes
>> depends on what happens to be on the stack.
>
> Nit: that last sentence is a bit on the verbose side, as it doesn't
> really give any more useful information. I'd just drop it.

Yes, agreed. Just me over explaining. Will keep that in mind for future
messages on the list.


>>  	test_commit -C foo x &&
>
> I think overall this looks good enough already, and the nits I have
> raised do not really need to be addressed.
>
> Thanks!
>
> Patrick
>
Thank you!
Olof

Re: [PATCH] bundle: do not free uninitialized memory on bad arguments

[Harald Nordgren]

> -       struct rev_info revs, revs_copy;
> +       struct rev_info revs, revs_copy = { 0 };

I'm not familiar with this code, but the fix seems simple enough and
has a test. Looks good.


Harald

Re: [PATCH] bundle: do not free uninitialized memory on bad arguments

[Olof Fairbrook]

On Mon, 19 Oct 2026, Harald Nordgren wrote:

>> -       struct rev_info revs, revs_copy;
>> +       struct rev_info revs, revs_copy = { 0 };
>
> I'm not familiar with this code, but the fix seems simple enough and
> has a test. Looks good.
>
>
> Harald
>

Thanks for the review!
Olof

Re: [PATCH] bundle: do not free uninitialized memory on bad arguments

[Junio C Hamano]

Olof Fairbrook <olof.fairbrook@gmail.com> writes:

> -	struct rev_info revs, revs_copy;
> +	struct rev_info revs, revs_copy = { 0 };

Obviously a sane and sensible thing to do.

Will queue.  Thanks.