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

[Christian Couder]

On Mon, Feb 26, 2018 at 3:06 PM, Derrick Stolee  wrote:
>
> Christian: do you want to submit the patch, or should I put one together?

I'd rather have you put one together.

Thanks,
Christian.

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

[Derrick Stolee]

On 2/26/2018 5:23 AM, Christian Couder wrote:

On Mon, Feb 26, 2018 at 10:53 AM, Jeff King  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(, p, first);
+   warning("p->num_objects = %u, first = %u",
+   p->num_objects, first);
+   if (!nth_packed_object_oid(, p, first))
+   die("oops!");
 extend_abbrev_len(, mad);
 } else if (first < num - 1) {
 nth_packed_object_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(, p, first);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first))
+   extend_abbrev_len(, mad);
 } else if (first < num - 1) {
-   nth_packed_object_oid(, p, first + 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first + 1))
+   extend_abbrev_len(, mad);
 }
 if (first > 0) {
-   nth_packed_object_oid(, p, first - 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first - 1))
+   extend_abbrev_len(, 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(, p, first);
+   if (!match && nth_packed_object_oid(, p, first))
    extend_abbrev_len(, mad);
-   } else if (first < num - 1) {
-   

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

[Christian Couder]

On Mon, Feb 26, 2018 at 10:53 AM, Jeff King  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(, p, first);
> +   warning("p->num_objects = %u, first = %u",
> +   p->num_objects, first);
> +   if (!nth_packed_object_oid(, p, first))
> +   die("oops!");
> extend_abbrev_len(, mad);
> } else if (first < num - 1) {
> nth_packed_object_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).

> 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(, p, first);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first))
+   extend_abbrev_len(, mad);
} else if (first < num - 1) {
-   nth_packed_object_oid(, p, first + 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first + 1))
+   extend_abbrev_len(, mad);
}
if (first > 0) {
-   nth_packed_object_oid(, p, first - 1);
-   extend_abbrev_len(, mad);
+   if (nth_packed_object_oid(, p, first - 1))
+   extend_abbrev_len(, mad);
}
mad->init_len = mad->cur_len;
 }

seems to prevent valgrind from complaining when running t5616-partial-clone.sh.

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

[Jeff King]

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(, p, first);
+   warning("p->num_objects = %u, first = %u",
+   p->num_objects, first);
+   if (!nth_packed_object_oid(, p, first))
+   die("oops!");
extend_abbrev_len(, mad);
} else if (first < num - 1) {
nth_packed_object_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

Use of uninitialised value of size 8 in sha1_name.c

[Christian Couder]

Hi Derrick,

These days when running:

./t5616-partial-clone.sh --valgrind

on master, I get a bunch of:

==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.

Best,
Christian.