Assertion `(U)bits < (int)safeint_internal::int_traits< T >::bitCount' failed.

[awelzel]

Running the following code through spicyc -d -j triggers an assertion. Haven't looked further, but seems the expression should fit 64bit, or a reasonable RuntimeError raised.

$ cat ./issues/sh.spicy 
module sh;

global first = uint8(167);
global rem = vector<uint8>(84, 144, 87);

global saslLen: uint64;

saslLen = (first << 24) + (rem[0] << 16) + (rem[1] << 8) + rem[2];

print "saslLen", saslLen;

$ ./build/bin/spicyc -j ./issues/sh.spicy  -d
spicyc: /home/awelzel/corelight-oss/spicy/hilti/runtime/include/hilti/rt/3rdparty/SafeInt/SafeInt.hpp:6007: constexpr SafeInt<T, E> SafeInt<T, E>::operator<<(SafeInt<U, E>) const [with U = long unsigned int; T = unsigned char; E = hilti::rt::integer::detail::SafeIntException]: Assertion `(U)bits < (int)safeint_internal::int_traits< T >::bitCount' failed.
Aborted (core dumped)

Without -d, it prints saslLen, 87 which seems to indicate only rem[2] is taken into account? The other uint8 values are all shifted to 0? Is this something one needs to worry about, or should shifting promote they type? A better result comes with:

(uint32(first) << 24) + (uint32(rem[0]) << 16) + (uint32(rem[1]) << 8) + uint32(rem[2]);

---

This was found while fuzzing Zeek's LDAP analyzer locally. It employs such a shift here:

  rem: uint8[3] if ( ctx.messageMode == MessageMode::ENCRYPTED && (|self.mech| == 0 || self.mech.starts_with(b"GSS")) ) {
    self.saslLen = (self.first << 24) + ($$[0] << 16) + ($$[1] << 8) + $$[2];
  }

Zeek's fuzzers are build with assert() enabled.

[bbannier]

The issue here is that we do not cleanly reject shifts beyond the width of an integer; instead this might be rejected by an assertion in SafeInt when building in debug mode. We should reject this cleanly for all build types at runtime via an exception.

All these cases are expected to fail:

print uint8(0)<<8;
print uint16(0)<<16;
print uint32(0)<<32;
print uint64(0)<<64;

print int8(0)<<8;
print int16(0)<<16;
print int32(0)<<32;
print int64(0)<<64;

Even though there is deliberate handling for undefined shifts in SafeInt I filed dcleblanc/SafeInt#63 to see whether they could be convinced to switch to a throwing versions.

[awelzel]

at runtime via an exception.

If the shift is by a const, it would be nice to get a compiler error at build time. Nice to have, but thought I'd comment :-)

I'm still wondering if << promoting to uint64 could be a feature, but that might just open a can of worms.

[bbannier]

I'm still wondering if << promoting to uint64 could be a feature, but that might just open a can of worms.

Yeah, for one there are no automatic coercions from uint64 to smaller integer types so this quickly can become noisy. The other issue is that even with an uint64 we could still shift by at most 64, but we have no integer type with that range and would need a runtime check regardless.

[bbannier]

Upstream has a PR open to move unsafe bitfshifts from assertion to exception. If this lands soon a bump would fix our issue (would still be good to add some tests); if it does not we could hook into the existing mechanism, see here.

I haven't audited which other places invoke SAFEINT_ASSERT, we might want to instrument it to throw as well (might require defining SAFEINT_REMOVE_NOTHROW.

[rsmmr]

Do we know if SafeInt is going to merge this?

[bbannier]

I pinged upstream on whether they could merge their fix. I also tried to customize SAFEINT_ASSERT like documented so we throw instead of abort, but this applies everywhere and is a pretty blunt tool, see #1897.