On Fri, Sep 22, 2017 at 12:11:59PM -0400, Jeff King wrote: > 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. Oh, nevermind. I managed to confuse myself again. The return value from hexval() is already unsigned. It's hex2chr that has the funny signed return. So the signedness is fine. > 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. This is still a bug, though. -Peff
