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.

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!

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

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.

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

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

⇐ 2022-07-15
⇐ hg64: a 64-bit histogram data structure
⇐
⇒ hg64 multithreaded histograms
⇒ 2022-10-12
⇒