From: Jeff King <peff@peff.net>
Date: Fri, 22 Sep 2017 16:11:59 GMT
Subject: Re: [PATCH 3/4] cache.h: hex2chr() - avoid -Wsign-compare warnings
Message-ID: <20170922161159.hkw4id4kpnwo7jou@sigill.intra.peff.net>
In-Reply-To: <89beb424-2cc0-bfe3-7847-c11035bfec70@ramsayjones.plus.com>

On Fri, Sep 22, 2017 at 05:05:03PM +0100, Ramsay Jones wrote:

> > As an aside, I also see some uses of hexval() that don't appear to be
> > quite as rigorous in checking for invalid characters. A few
> > unconditionally shift the first nibble and assume that there will still
> > be high bits set. I think that's generally true for twos-complement
> > negative numbers, but isn't shifting off the left side of a signed
> > integer undefined behavior?
> 
> All uses of hexval() that I can see are shifting an unsigned value.
> Have I missed something?

Hmm. get_hex_color() does:

  unsigned int val;
  val = (hexval(in[0]) << 4) | hexval(in[1]));

Isn't that shifting the signed return value of hexval(), and then
converting it to unsigned afterwards?

I've been confused by C's integer conversion rules before, though, so
perhaps I'm wrong.

I think if this function is fed an empty string that it will also read
past the end of the buffer for in[1]. It shouldn't matter, since the NUL
in in[0] would cause us to return an error regardless, but it's still
undefined behavior.

In fact, this whole function is just hex2chr() implemented badly. Who is
responsible for this terrible code? ;)

-Peff