Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
awelzel opened this issue Aug 6, 2024 · 6 comments · May be fixed by #1897
Open

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

awelzel opened this issue Aug 6, 2024 · 6 comments · May be fixed by #1897
Labels
Bug Something isn't working Good first issue Good for newcomers Runtime Library Issues related to the HILTI or Spicy runtime libraries

Comments

@awelzel
Copy link
Contributor

awelzel commented Aug 6, 2024

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 bbannier added Bug Something isn't working Good first issue Good for newcomers labels Aug 6, 2024
@bbannier
Copy link
Member

bbannier commented Aug 7, 2024

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.

@bbannier bbannier added the Runtime Library Issues related to the HILTI or Spicy runtime libraries label Aug 7, 2024
awelzel added a commit to zeek/zeek that referenced this issue Aug 7, 2024
@awelzel
Copy link
Contributor Author

awelzel commented Aug 7, 2024

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
Copy link
Member

bbannier commented Aug 7, 2024

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
Copy link
Member

bbannier commented Aug 9, 2024

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
Copy link
Member

rsmmr commented Oct 17, 2024

Do we know if SafeInt is going to merge this?

@bbannier
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Good first issue Good for newcomers Runtime Library Issues related to the HILTI or Spicy runtime libraries
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants