Re: [PATCH] wt-status: use strncmp() for length-limited string comparison

From: René Scharfe <l.s.r@web.de>
Date: Nov 25, 2015, 02:16 UTC
Message-ID: <56551A11.9030809@web.de>
In-Reply-To: <20151124213601.GB29185@sigill.intra.peff.net>

Am 24.11.2015 um 22:36 schrieb Jeff King:

Show 18 quoted lines

> On Fri, Nov 06, 2015 at 11:47:03PM +0100, René Scharfe wrote:
> 
>> When a branch name is longer than four characters, memcmp() can read
>> past the end of the string literal "HEAD".  Use strncmp() instead, which
>> stops at the end of a string.  This fixes the following test failures
>> with AddressSanitizer:
> 
> Hmm. I think this is mostly harmless, as a comparison like:
> 
>    memcmp("HEAD and more", "HEAD", strlen("HEAD"))
> 
> would yield non-zero when we compare the NUL in the second string to
> whatever is in the first. So I assume what is going on is that memcmp is
> doing larger compares than byte by byte, and is examining 4 or 8 bytes
> starting at that NUL.
> 
> The outcome is equivalent, but we do touch memory that is not ours, so I
> think this is a positive direction in that sense.

Yes, except it should be strlen("HEAD and more") in your example code; with strlen("HEAD") it would compare just 4 bytes and return 0.

Show 14 quoted lines

> But...
> 
>> diff --git a/wt-status.c b/wt-status.c
>> index 435fc28..8dc281b 100644
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
>>   	hashcpy(cb->nsha1, nsha1);
>>   	for (end = target; *end && *end != '\n'; end++)
>>   		;
>> -	if (!memcmp(target, "HEAD", end - target)) {
>> +	if (!strncmp(target, "HEAD", end - target)) {
> 
> This will match prefixes like "HEA" in the target, won't it?

Oww, yes. :-/  NB: The existing code does the same.

Show 10 quoted lines

> I think you want something more like:
> 
>    if (end - target == 4 && !memcmp(target, "HEAD", 4))
> 
> I tried to think of a way that didn't involve a magic number. The best I
> came up with is:
> 
>    if (skip_prefix(target, "HEAD", &v) && v == end)
> 
> but that requires an extra variable, and is arguably more obfuscated.

Using one more variable isn't that bad, as long as it gets a fitting name. Or we could reuse "end" (I'm not worrying about scanning "HEAD" twice very much):

diff --git a/wt-status.c b/wt-status.c
index 435fc28..96a731e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
 	target += strlen(" to ");
 	strbuf_reset(&cb->buf);
 	hashcpy(cb->nsha1, nsha1);
-	for (end = target; *end && *end != '\n'; end++)
-		;
-	if (!memcmp(target, "HEAD", end - target)) {
+	if (skip_prefix(target, "HEAD", &end) && (!*end || *end == '\n')) {
 		/* HEAD is relative. Resolve it to the right reflog entry. */
 		strbuf_addstr(&cb->buf,
 			      find_unique_abbrev(nsha1, DEFAULT_ABBREV));
 		return 1;
 	}
+	for (end = target; *end && *end != '\n'; end++)
+		;
 	strbuf_add(&cb->buf, target, end - target);
 	return 1;
 }