git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Christian Couder <christian.couder@gmail.com>, Jeff King <peff@peff.net>
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	dstolee@microsoft.com
Subject: Re: Use of uninitialised value of size 8 in sha1_name.c
Date: Mon, 26 Feb 2018 09:06:27 -0500	[thread overview]
Message-ID: <0a85ea3b-3f64-f67d-b4d5-a761cbc4c6db@gmail.com> (raw)
In-Reply-To: <CAP8UFD01wkpNuXSHxQTETi3+tWBM0E=iYXQeT2r7tGs=2Yq2EA@mail.gmail.com>

On 2/26/2018 5:23 AM, Christian Couder wrote:
> On Mon, Feb 26, 2018 at 10:53 AM, Jeff King <peff@peff.net> wrote:
>> On Mon, Feb 26, 2018 at 10:04:22AM +0100, Christian Couder wrote:
>>
>>> ==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!
> Yeah, I tried to t5601-clone.sh with --valgrind and I also get one
> error (the same "Uninitialised value" error actually).

Thanks for finding this. Sorry for the bug.

>> 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.
> Yeah, something like:
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 611c7d24dd..a041d8d24f 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -547,15 +547,15 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
>           */
>          mad->init_len = 0;
>          if (!match) {
> -               nth_packed_object_oid(&oid, p, first);
> -               extend_abbrev_len(&oid, mad);
> +               if (nth_packed_object_oid(&oid, p, first))
> +                       extend_abbrev_len(&oid, mad);
>          } else if (first < num - 1) {
> -               nth_packed_object_oid(&oid, p, first + 1);
> -               extend_abbrev_len(&oid, mad);
> +               if (nth_packed_object_oid(&oid, p, first + 1))
> +                       extend_abbrev_len(&oid, mad);
>          }
>          if (first > 0) {
> -               nth_packed_object_oid(&oid, p, first - 1);
> -               extend_abbrev_len(&oid, mad);
> +               if (nth_packed_object_oid(&oid, p, first - 1))
> +                       extend_abbrev_len(&oid, mad);
>          }
>          mad->init_len = mad->cur_len;
>   }
>
> seems to prevent valgrind from complaining when running t5616-partial-clone.sh.

This seems like the safest fix, but also we could use our values of 
"match", "first" and "num" to safely call nth_packed_object_oid(). 
However, since nth_packed_object_oid() checks the bounds, don't 
duplicate work and just use the return value.

I would reformat your patch slightly, but that's just preference:

diff --git a/sha1_name.c b/sha1_name.c
index 611c7d2..97b632c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -546,17 +546,14 @@ static void find_abbrev_len_for_pack(struct 
packed_git *p,
          * nearby for the abbreviation length.
          */
         mad->init_len = 0;
-       if (!match) {
-               nth_packed_object_oid(&oid, p, first);
+       if (!match && nth_packed_object_oid(&oid, p, first))
                 extend_abbrev_len(&oid, mad);
-       } else if (first < num - 1) {
-               nth_packed_object_oid(&oid, p, first + 1);
+       else if (first < num - 1 &&
+                nth_packed_object_oid(&oid, p, first + 1))
                 extend_abbrev_len(&oid, mad);
-       }
-       if (first > 0) {
-               nth_packed_object_oid(&oid, p, first - 1);
+       if (first > 0 && nth_packed_object_oid(&oid, p, first - 1))
                 extend_abbrev_len(&oid, mad);
-       }
+
         mad->init_len = mad->cur_len;
  }

Christian: do you want to submit the patch, or should I put one together?

Thanks,
-Stolee


  reply	other threads:[~2018-02-26 14:06 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-26  9:04 Use of uninitialised value of size 8 in sha1_name.c Christian Couder
2018-02-26  9:53 ` Jeff King
2018-02-26 10:23   ` Christian Couder
2018-02-26 14:06     ` Derrick Stolee [this message]
2018-02-26 14:43       ` Christian Couder
2018-02-26 14:56         ` [PATCH] sha1_name: fix uninitialized memory errors Derrick Stolee
2018-02-26 20:41           ` Jeff King
2018-02-27 11:47             ` [PATCH v2] " Derrick Stolee
2018-02-27 21:33               ` Jeff King
2018-02-27 22:30                 ` Junio C Hamano
2018-02-27 22:40                   ` Jeff King
2018-02-28 20:50               ` Junio C Hamano
2018-02-28 20:57                 ` Derrick Stolee
2018-02-28 21:06                   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0a85ea3b-3f64-f67d-b4d5-a761cbc4c6db@gmail.com \
    --to=stolee@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).