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

[Patrick Steinhardt <ps@pks.im>]

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