Am 24.11.2015 um 22:36 schrieb Jeff King:
> 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.
> 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.
> 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;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html