Re: [PATCH 06/11] pack-redundant: avoid using uninitialized memory

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2022, 04:53 UTC
Message-ID: <xmqqilp1w6oi.fsf@gitster.g>
In-Reply-To: <bc29a9710e3a22e6d660098c4f201f3bfecc54ea.1655336146.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 24 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the `add_pack()` function, we forgot to initialize the field `next`,
> which could potentially lead to readin uninitialized memory later.
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/pack-redundant.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index ed9b9013a5f..1f7da1f68b6 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c
> @@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p)
>  	}
>  	l.all_objects_size = l.remaining_objects->size;
>  	l.unique_objects = NULL;
> +	l.next = NULL;
>  	if (p->pack_local)
>  		return pack_list_insert(&local_packs, &l);
>  	else

		return pack_list_insert(&altodb_packs, &l);

The pack_list_insert reads like so:

static inline struct pack_list * pack_list_insert(struct pack_list **pl,
					   struct pack_list *entry)
{
	struct pack_list *p = xmalloc(sizeof(struct pack_list));
	memcpy(p, entry, sizeof(struct pack_list));
	p->next = *pl;
	*pl = p;
	return p;
}

IOW, we prepare members of "l", pass it as "entry" to pack_list_insert(), but the helper function allocates a new piece of memory, copies "l" to it, and then sets the .next member of it to a reasonable value.

And after that, the "l" that was merely used as a template goes out of scope.

So I think this is a false positive. memcpy(p) does copy the uninitialized garbage that is held in entry->next to p->next, but it is overwritten with a reasonable address immediately after that.