On Mon, Feb 26, 2018 at 10:53 AM, Jeff King <peff@xxxxxxxx> 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).
> 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.
