This week I am on “escalation engineer” duty, which means triaging new
bug reports. One of them was found by ubsan (
reporting an overflow in a signed integer shift. This was fun to fix
because it needed a bit of lateral thinking.
C’s bit shift operators require some care to avoid undefined behaviour. One of the pitfalls is that a shift is not allowed to change the sign bit in a signed integer.
It was exactly this problem that ubsan was reporting.
The problem code was in a portability wrapper around
and the bug depended on which platform it was compiled for. The
problem code turned out to be 21 years old!
BIND has an
isc_resourcevalue_t which is always a
portability wrapper needs to convert it to the platform’s
protects against overflow in the conversion by saturating, setting the
limit to its maximum value instead.
In principle, that ought to be straightforward, but we don’t know how
rlim_t is (it might be 32 bits or 64 bits) or whether it is
signed or not. And there isn’t an
RLIM_MAX constant for us to use to
check for overflow. We can’t use
RLIM_INFINITY because that might be
a special out-of-range value like
rlim_t to be unsigned, but some unixes with long
histories have kept it signed for compatibility with old code, and
those platforms were where ubsan kicked up a fuss.)
The buggy part of the old code was:
bool rlim_t_is_signed = (((double)(rlim_t)-1) < 0); if (rlim_t_is_signed) rlim_max = ~((rlim_t)1 << (sizeof(rlim_t) * 8 - 1));
This shifts 1 into the sign bit (a no-no!) then inverts it to get the
To avoid the problem, I needed to stay
unsigned as much as possible.
I can use an
isc_resourcevalue_t which is unsigned and (should be!)
at least as large as
rlim_t. So, instead of asking how big an
rlim_t is, the new question is how much bigger is an
We can count how many bytes wider our
rlim_max variable is (should
be 0 or 4) and whether
rlim_t has a sign bit. (I couldn’t think of a
better way to work that out.)
isc_resourcevalue_t rlim_max = UINT64_MAX; size_t wider = sizeof(rlim_max) - sizeof(rlim_t); bool sign_bit = (double)(rlim_t)-1 < 0;
Then we just need to shift the excess bits out of
rlim_max to get
the result we want.
rlim_max >>= CHAR_BIT * wider + sign_bit;
Then we can convert the resource limit value with overflow protection.
rlim_value = ISC_MIN(value, rlim_max);
This change replaced 18 lines of code with 8 lines. I like patches with negative net LoC :-)