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

From: Jeff King <peff@peff.net>
Date: Nov 24, 2015, 21:36 UTC
Message-ID: <20151124213601.GB29185@sigill.intra.peff.net>
In-Reply-To: <563D2DE7.1030005@web.de>

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.

But...

Show 10 quoted lines

> 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?

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.

-Peff