Subject: Re: [PATCH v3 13/22] resolve_ref(): turn buffer into a proper string as soon as possible

From: Michael Haggerty <[email-masked]>
Date: Fri, 23 Sep 2011 15:11:25 +0200

On 09/23/2011 10:17 AM, Thomas Rast wrote:
> Michael Haggerty wrote:
>> Immediately strip off trailing spaces and null-terminate the string
>> holding the contents of the reference file; this allows the use of
>> string functions and avoids the need to keep separate track of the
>> string's length.  (get_sha1_hex() fails automatically if the string is
>> too short.)
>>
>> Signed-off-by: Michael Haggerty <[email-masked]>
> 
> I'm getting valgrind failures in t1450-fsck and t3800-mktag which
> blame to this commit.  For t1450 it looks as follows:
> 
>     ok 5 - object with bad sha1
> 
>     expecting success: 
>             git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&
>             test_when_finished "git update-ref -d refs/heads/invalid" &&
>             git fsck 2>out &&
>             cat out &&
>             grep "not a commit" out
> 
>     ==19623== Use of uninitialised value of size 8
>     ==19623==    at 0x4B6747: hexval (cache.h:798)
>     ==19623==    by 0x4B6797: get_sha1_hex (hex.c:42)
>     ==19623==    by 0x4DD12A: resolve_ref (refs.c:588)

Thanks for finding this.

The problem comes from passing get_sha1_hex() an arbitrary
NUL-terminated string.  get_sha1_hex() calls hexval(), which returns -1
if it finds a NUL character, and therefore (correctly) rejects any
string that is less than 40 characters.  The problem is that
get_sha1_hex() reads characters pairwise, and even if the first
character in a pair is NUL, the second character is read anyway.
Therefore, for strings whose strlen is an even number less than 40,
get_sha1_hex() reads past the terminating NUL.

I don't know whether get_sha1_hex() is *supposed* to work for arbitrary
NUL-terminated strings because it is not documented.  But other callers
seem to make the same assumption that I did, so I think that I will fix
get_sha1_hex() to not read past a NUL value (albeit at the cost of an
extra check in each iteration of the loop).

The fix is trivial and will be sent shortly.

Michael

-- 
Michael Haggerty
[email-masked]
http://softwareswirl.blogspot.com/