On Thu, Sep 21, 2017 at 05:48:38PM +0100, Ramsay Jones wrote:
> diff --git a/cache.h b/cache.h
> index a916bc79e..a0e3e362c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1243,8 +1243,8 @@ static inline unsigned int hexval(unsigned char c)
> */
> static inline int hex2chr(const char *s)
> {
> - int val = hexval(s[0]);
> - return (val < 0) ? val : (val << 4) | hexval(s[1]);
> + unsigned int val = hexval(s[0]);
> + return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
> }
Ironically, the unsigned return from hexval() comes from internally
converting the signed char in hexval_table. And then we again return it
as a signed int from hex2chr().
Would it make sense to return a signed int from hexval()? That would
make hex2chr just work as it tries to above. I admit that shifting
signed values is a little funny, but it should be fine here since we
know they're no larger than 8 bits in the first place.
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?
And mailinfo's decode_q_segment() does not seem to check for errors at
all.
Handling those is getting far off your original patch, but I'm having
trouble figuring out if it's saner for us to consistently stick to
all-signed or all-unsigned here.
-Peff
