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

Commit Message

Johannes Schindelin June 15, 2022, 11:35 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Junio C Hamano June 16, 2022, 4:53 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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.

Patch

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