This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][ubsan] Implicit Conversion Sanitizer - integer sign change - compiler-rt part
ClosedPublic

Authored by lebedev.ri on Aug 3 2018, 6:22 AM.

Diff Detail

Event Timeline

lebedev.ri created this revision.Aug 3 2018, 6:22 AM

Rebased ontop of revised D50250.

Rebased ontop of revised D50250.

Rebased, ping.

lebedev.ri planned changes to this revision.Aug 17 2018, 9:37 AM

Depends on D50902.
(which should land first, ideally.)

Rebased ontop of D50902.

Ping once again :)

Ping.
The prerequisite "split truncation sanitizer into unsigned and signed cases" has landed.
Would be awesome to get this going :)

And another ping.

Rebased.
The clang part D50250 finally got reviewed, so i'm wondering if someone could review this, please? :)

lebedev.ri changed the repository for this revision from rCTE Clang Tools Extra to rCRT Compiler Runtime.Oct 24 2018, 12:32 PM
lebedev.ri removed a subscriber: cfe-commits.

The non-test bits look really straightforward to me.

I have minor questions in inline comments, but sorry, I'll request a big change :-/

Please match the types of casts and parameters. You're often matching uint32_t to unsigned int (also a signed char with int8_t (not as problematic here, though), etc). We don't have a guarantee, and the "assuming int is 32-bit" part might cause problems eventually. I'm ok with either version:

  • Always use sized types like uint32_t
  • Always use "platform types" like unsigned int (or whatever the standard names them)

Otherwise, this looks good.

Thank you,
Filipe

P.S: Yay, I managed not to ask about the ubsan-standalone feature! (sorry about that in the earlier reviews :-) )

lib/ubsan/ubsan_handlers.cc
467

This means *both* errors got triggered, right? Or can we have this one with just one being triggered?
If it's always both, I'd rather have an And instead of Or in its name.

test/fuzzer/ImplicitSignedIntegerTruncationOrSignChangeTest.cpp
13 ↗(On Diff #170953)

Please cast it to the same type. An unsigned int is not necessarily the same as a uint32_t.

I have minor questions in inline comments, but sorry, I'll request a big change :-/

Please match the types of casts and parameters. You're often matching uint32_t to unsigned int (also a signed char with int8_t (not as problematic here, though), etc). We don't have a guarantee, and the "assuming int is 32-bit" part might cause problems eventually. I'm ok with either version:

  • Always use sized types like uint32_t
  • Always use "platform types" like unsigned int (or whatever the standard names them)

Otherwise, this looks good.

Yay, thank you for the feedback!

Thank you,
Filipe

P.S: Yay, I managed not to ask about the ubsan-standalone feature! (sorry about that in the earlier reviews :-) )

:)

lib/ubsan/ubsan_handlers.cc
467

We emit ICCK_SignedIntegerTruncationOrSignChange in the case of truncation (src len > dst len) from unsigned to signed.
We could do this as a two *separate* checks:

  • truncation (and emit ICCK_SignedIntegerTruncation) All the truncated bits should be the same as the sign bit of the dst.
  • sign change (ICCK_IntegerSignChange) The src is unsigned (i.e. implicitly non-negative), therefore the dst should be non-negative too. I.e., the dst sign bit should be unset.

But then we do two checks for per one implicit cast, which results in code bloat, etc.
But if we combine these two checks, we end up with:
All the truncated bits should be the same as the sign bit of the dst AND the dst sign bit should be unset.
Which is optimized down to src <= std::numeric_limits<decltype(dst)>::max().

You can have a cast which is sign-changing, but does not result in a truncation:
unsigned(0b1111) -> char(0b11) - it's not a truncation, all the bits are the same, but it is a sign change.

So no, this really is an or.

filcab added inline comments.Oct 30 2018, 6:23 AM
lib/ubsan/ubsan_handlers.cc
467

Sounds good. My only request is the types being consistent then. Otherwise LGTM!

lebedev.ri marked an inline comment as done.

Rebased, changed types in ImplicitSignedIntegerTruncationOrSignChangeTest fuzzer test.
I will do the rest of type streamlining in a separate review.

filcab accepted this revision.Oct 30 2018, 11:59 AM

LGTMing.

This revision is now accepted and ready to land.Oct 30 2018, 11:59 AM

LGTMing.

Thank you for the review!