Re: Use of uninitialised value of size 8 in sha1_name.c

From: Jeff King <peff@peff.net>
Date: Feb 26, 2018, 09:53 UTC
Message-ID: <20180226095311.GA14831@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD23z9YDukO=O+cK=o_0DLcxbkXWzp4rCA1kRXGTZ-TMcQ@mail.gmail.com>

On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:

Show 22 quoted lines

> ==21455== Use of uninitialised value of size 8
> ==21455==    at 0x2D2A73: get_hex_char_from_oid (sha1_name.c:492)
> ==21455==    by 0x2D2AFE: extend_abbrev_len (sha1_name.c:502)
> ==21455==    by 0x2D2C3D: find_abbrev_len_for_pack (sha1_name.c:551)
> ==21455==    by 0x2D2CFF: find_abbrev_len_packed (sha1_name.c:569)
> ==21455==    by 0x2D2E12: find_unique_abbrev_r (sha1_name.c:608)
> ==21455==    by 0x2DCB66: strbuf_add_unique_abbrev (strbuf.c:877)
> ==21455==    by 0x14F7CE: update_local_ref (fetch.c:700)
> ==21455==    by 0x1500CF: store_updated_refs (fetch.c:871)
> ==21455==    by 0x15035B: fetch_refs (fetch.c:932)
> ==21455==    by 0x150CF8: do_fetch (fetch.c:1146)
> ==21455==    by 0x1515AB: fetch_one (fetch.c:1370)
> ==21455==    by 0x151A1D: cmd_fetch (fetch.c:1457)
> ==21455==  Uninitialised value was created by a stack allocation
> ==21455==    at 0x2D2B2E: find_abbrev_len_for_pack (sha1_name.c:513)
> ==21455==
> 
> A quick git blame seems to point to 0e87b85683 (sha1_name: minimize
> OID comparisons during disambiguation, 2017-10-12).
> 
> It is difficult to tell for sure though as t5616-partial-clone.sh was
> added after that commit.

I think that commit is to blame, though the error isn't exactly where that stack trace puts it. Try this:

diff --git a/sha1_name.c b/sha1_name.c
index 611c7d24dd..6f7f36436f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -547,7 +547,10 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
 	 */
 	mad->init_len = 0;
 	if (!match) {
-		nth_packed_object_oid(&oid, p, first);
+		warning("p->num_objects = %u, first = %u",
+			p->num_objects, first);
+		if (!nth_packed_object_oid(&oid, p, first))
+			die("oops!");
 		extend_abbrev_len(&oid, mad);
 	} else if (first < num - 1) {
 		nth_packed_object_oid(&oid, p, first + 1);

I get failures all over the test suite, like this:

  warning: p->num_objects = 4, first = 3
  warning: p->num_objects = 8, first = 6
  warning: p->num_objects = 10, first = 0
  warning: p->num_objects = 4, first = 0
  warning: p->num_objects = 8, first = 0
  warning: p->num_objects = 10, first = 10
  fatal: oops!

Any time the abbreviated hex would go after the last object in the pack,
then first==p->num_objects, and nth_packed_object_oid() will fail. That
leaves uninitialized data in "oid", which is what valgrind complains
about when we examine it in extend_abbrev_len().

Most of the time the code behaves correctly anyway, because the
uninitialized bytes are unlikely to match up with our hex and extend the
length. Probably we just need to detect that case and skip the call to
extend_abbrev_len() altogether.

-Peff