How big is this integer type?

[fanf]

https://dotat.at/@/2022-09-23-ub.html

This week I am on "escalation engineer" duty, which means triaging new bug reports. One of them was found by ubsan (-fsanitize=undefined) reporting an overflow in a signed integer shift. This was fun to fix because it needed a bit of lateral thinking.

treacherous shifts

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 setrlimit(), and the bug depended on which platform it was compiled for. The problem code turned out to be 21 years old!

resource limits

BIND has an isc_resourcevalue_t which is always a uint64_t. The portability wrapper needs to convert it to the platform's rlim_t. It 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 big 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 -1.

(POSIX requires 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.)

what not to do

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 maximum positive rlim_t value.

fixed version

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

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);

bonus

This change replaced 18 lines of code with 8 lines. I like patches with negative net LoC :-)

Comments

[simont]

This is one of the moments where a spot of C++ would have actually been helpful, because you could have just said std::numeric_limits<rlim_t>::max() and been done with it.

(Of course probably inside the <limits> header some similar trickery is going on to implement that. But at least that way, if anyone discovers a bug or generality problem in the trickery, only the header has to be fixed and not everyone's application code!)

An alternative point fix to the single line containing the UB shift might have been:

rlim_max = 2 * (((rlim_t)1 << (sizeof(rlim_t) * 8 - 2)) - 1) + 1;

in which, instead of shifting 1 into the sign bit, we shift it one bit less far so that it ends up just below the sign bit. If that's x, then the answer we want is 2x-1, which we compute as 2(x-1)+1 so that the number we're doubling doesn't quite overflow.

(But if your larger fix also made the rest of the code smaller, I'm not going to argue that you should have done my thing instead!)

[hilarita]

I'd certainly want that single line with a comment attached to it, so the next engineer doesn't have to work out any of this from scratch ;-)

[fanf]

When fixing something like this, which is both tricky and wrong, I try to make it simpler as well as more correct :-)

[bens_dad]

I'm wary of signed-ness and left shift after having an unsigned char type-promoted to a signed int and then shifted :-(