Page MenuHomePhabricator

[ubsan] Teach the pointer overflow check that "p - <unsigned> <= p" (PR33430)
ClosedPublic

Authored by vsk on Jun 12 2017, 3:23 PM.

Details

Summary

The pointer overflow check gives false negatives when dealing with
expressions in which an unsigned value is subtracted from a pointer.
This is summarized in PR33430 [1]: ubsan permits the result of the
subtraction to be greater than "p", but it should not.

To fix the issue, we should track whether or not the pointer expression
is a subtraction. If it is, and the indices are unsigned, we know to
expect "p - <unsigned> <= p".

I've tested this by running check-{llvm,clang} with a stage2 ubsan-enabled build. I've also added some tests to compiler-rt, which I'll upload in a separate patch.

[1] https://bugs.llvm.org/show_bug.cgi?id=33430

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Jun 12 2017, 3:23 PM
efriedma edited edge metadata.Jun 12 2017, 3:33 PM

Looks okay, but I haven't been reviewing this series of patches closely, so I could be missing something.

lib/CodeGen/CGExprScalar.cpp
3974 ↗(On Diff #102250)

Please make this an "else" rather than an "else if" (you can assert the inside the else body if it's helpful).

vsk updated this revision to Diff 102261.Jun 12 2017, 4:39 PM
vsk marked an inline comment as done.
vsk edited the summary of this revision. (Show Details)

Thanks for the review! I'll wait for another 'lgtm'.

dtzWill edited edge metadata.Jun 13 2017, 1:04 PM

Don't mean to block this, but just FYI I won't be able to look into this carefully until later this week (sorry!).

Kicked off a rebuild using these patches just now, though! O:)

vsk added a comment.Jun 13 2017, 3:11 PM

Don't mean to block this, but just FYI I won't be able to look into this carefully until later this week (sorry!).

Kicked off a rebuild using these patches just now, though! O:)

No problem, thanks for taking a look.

aprantl added inline comments.
lib/CodeGen/CGExpr.cpp
3010 ↗(On Diff #102261)

Might want to define an

enum { NotSubtraction = false, IsSubtraction = true } in the header. We do this in other places.

vsk updated this revision to Diff 102603.Jun 14 2017, 1:53 PM
vsk marked an inline comment as done.

Address Adrian's comment about using an enum to simplify some calls.

vsk updated this revision to Diff 103606.Jun 22 2017, 11:25 AM

Fix a typo introduced in emitArraySubscriptGEP while refactoring /*isSubtraction=false*/ to CodeGenFunction::NotSubtraction, and add CHECK lines which catch the issue.

vsk added a comment.Jul 12 2017, 12:36 PM

@dtzWill do you have any further comments on this one?

I'd like to get another 'lgtm' before committing, and it'd be nice to get this in before llvm 5.0 branches (7/19).

FWIW we've been living on this for a few weeks internally without any issues:
https://github.com/apple/swift-clang/commit/3ebe7d87b9d545aebdd80452d0b79695ff871bce

In D34121#806978, @vsk wrote:

@dtzWill do you have any further comments on this one?

I'd like to get another 'lgtm' before committing, and it'd be nice to get this in before llvm 5.0 branches (7/19).

FWIW we've been living on this for a few weeks internally without any issues:
https://github.com/apple/swift-clang/commit/3ebe7d87b9d545aebdd80452d0b79695ff871bce

@vsk sorry for the delay. Looks solid to me, seems to work well in my testing.

Unrelated to the suitability of the patch itself, but on the subject:
Interestingly there don''t seem to be any changes in observed errors, which on one hand is great (yay no breakage and earlier results were mostly correct) but on the other isn't what I expected.
Does this match your experiences?

dtzWill accepted this revision.Jul 13 2017, 11:41 AM
This revision is now accepted and ready to land.Jul 13 2017, 11:41 AM
vsk added a comment.Jul 13 2017, 1:12 PM
In D34121#806978, @vsk wrote:

@dtzWill do you have any further comments on this one?

I'd like to get another 'lgtm' before committing, and it'd be nice to get this in before llvm 5.0 branches (7/19).

FWIW we've been living on this for a few weeks internally without any issues:
https://github.com/apple/swift-clang/commit/3ebe7d87b9d545aebdd80452d0b79695ff871bce

@vsk sorry for the delay. Looks solid to me, seems to work well in my testing.

Unrelated to the suitability of the patch itself, but on the subject:
Interestingly there don''t seem to be any changes in observed errors, which on one hand is great (yay no breakage and earlier results were mostly correct) but on the other isn't what I expected.
Does this match your experiences?

Thanks for testing the patch out! Your results match my experiences with some Apple frameworks. The 'p - unsigned > p' case seems to be relatively uncommon.