This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Update condition to avoid overflow.
ClosedPublic

Authored by asbirlea on Jul 29 2019, 1:55 PM.

Details

Summary

Update condition to remove addition that may cause an overflow.
Resolves PR42814.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Jul 29 2019, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2019, 1:56 PM
Herald added a subscriber: jlebar. · View Herald Transcript
lebedev.ri added inline comments.
lib/Analysis/ConstantFolding.cpp
550–552 ↗(On Diff #212220)

Is Offset==0 == 0'th byte of the constant?
If so, perhaps this check could simply be done first?

asbirlea updated this revision to Diff 212226.Jul 29 2019, 2:18 PM

Update per comment suggestion: reorder conditions.

asbirlea marked an inline comment as done.Jul 29 2019, 2:18 PM
asbirlea added a reviewer: lebedev.ri.
lebedev.ri added inline comments.Jul 29 2019, 2:30 PM
lib/Analysis/ConstantFolding.cpp
551 ↗(On Diff #212226)

This check looks suspicious to me.
I'd expect this to be if (Offset + BytesLoaded >= InitializerSize),
this way we are checking that BytesLoaded bytes lies within the global.

asbirlea marked an inline comment as done.Jul 29 2019, 2:42 PM
asbirlea added inline comments.
lib/Analysis/ConstantFolding.cpp
551 ↗(On Diff #212226)

If Offset can be a negative value, it's possible to get an OOB access when Offset + BytesLoaded <= 0.
We can also get an OOB access if (Offset + BytesLoaded >= InitializerSize), but the two checks seem orthogonal given this section of code (I'm not familiar with the larger scope of this code).

lebedev.ri marked an inline comment as done.Jul 29 2019, 2:46 PM
lebedev.ri added inline comments.
lib/Analysis/ConstantFolding.cpp
551 ↗(On Diff #212226)

Okay, sounds plausible.

fhahn added a subscriber: fhahn.Jul 30 2019, 2:43 AM
fhahn added inline comments.
lib/Analysis/ConstantFolding.cpp
551 ↗(On Diff #212226)

I think with the check as it is, there still could be an overflow *in theory*, e.g. for constants that are int64_max big. Given that we only allow global constants with initializers, this would mean a huge bitcode file, but while you're at it, it might be worth changing the comparisons to if (Offfset <= -BytesLoaded) as well

RKSimon added inline comments.Jul 30 2019, 5:32 AM
test/Transforms/SCCP/ubsan_overflow.ll
1 ↗(On Diff #212226)

Shouldn't this be

opt < %s -sccp -S
asbirlea updated this revision to Diff 212376.Jul 30 2019, 10:05 AM
asbirlea marked 3 inline comments as done.

Address comment.

test/Transforms/SCCP/ubsan_overflow.ll
1 ↗(On Diff #212226)

The behavior is the same.

asbirlea updated this revision to Diff 212428.Jul 30 2019, 1:45 PM

Added needed cast from unsigned to int64_t.

sanjoy.google edited reviewers, added: sanjoy.google; removed: sanjoy.Jul 30 2019, 3:06 PM
sanjoy.google added inline comments.Jul 30 2019, 3:11 PM
lib/Analysis/ConstantFolding.cpp
551 ↗(On Diff #212428)

Use static_cast? That also makes it obvious whether the cast happens before or after the multiply.

asbirlea updated this revision to Diff 212609.Jul 31 2019, 9:51 AM
asbirlea marked 2 inline comments as done.

Address comment.

sanjoy.google accepted this revision.Jul 31 2019, 10:59 AM

lgtm with some nits

lib/Analysis/ConstantFolding.cpp
547 ↗(On Diff #212609)

OOC is there a reason to flip the order of the two checks? If yes, can you please update the commit message (so as to not confuse folks digging through the commit history in the future)?

test/Transforms/SCCP/ubsan_overflow.ll
12 ↗(On Diff #212609)

We probably should remove the unreachable. As written the function has unconditional UB which means some other optimization within SCCP could kick in.

Probably the best thing to do is to return the value you loaded so that it can't get DCE'd away either (SCCP is unlikely to do that, but you never know).

This revision is now accepted and ready to land.Jul 31 2019, 10:59 AM
asbirlea updated this revision to Diff 212628.Jul 31 2019, 11:19 AM

Address comments.
Thank you for the review!

asbirlea marked 2 inline comments as done.Jul 31 2019, 11:20 AM
This revision was automatically updated to reflect the committed changes.