Page MenuHomePhabricator

Fix -fsanitize=array-bounds with comma operator
Needs ReviewPublic

Authored by vitalybuka on Apr 3 2020, 1:37 AM.

Details

Reviewers
rsmith
Summary

Depends on D77373.

Diff Detail

Event Timeline

vitalybuka created this revision.Apr 3 2020, 1:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 1:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vitalybuka updated this revision to Diff 254728.Apr 3 2020, 1:48 AM

remove debug dump

vitalybuka marked 2 inline comments as done.Apr 3 2020, 1:50 AM
vitalybuka added inline comments.
clang/test/CodeGen/bounds-checking.c
116

C version works even without patch

Harbormaster completed remote builds in B51598: Diff 254728.
vitalybuka updated this revision to Diff 255016.Apr 4 2020, 2:11 AM

try arc diff

rsmith added inline comments.Apr 8 2020, 6:49 PM
clang/lib/CodeGen/CGExpr.cpp
882–887

If we're going to further extend what Clang considers to be a flexible array access, we should do so consistently across our warning machinery and our sanitizers. Perhaps we could start by unifying this function with IsTailPaddedMemberArray in SemaChecking?

vitalybuka marked an inline comment as done.Apr 9 2020, 2:16 PM
vitalybuka added inline comments.
clang/lib/CodeGen/CGExpr.cpp
882–887

There is one place in external code which is blocking me from enabling this at Google.

How much work it's going to be? To me these functions looks very different.

rsmith added inline comments.Apr 22 2020, 12:57 PM
clang/lib/CodeGen/CGExpr.cpp
882–887

If you don't want to do the refactoring, please at least update Sema::CheckArrayAccess to skip over commas when looking for a member access in BaseExpr. Testcase:

struct X { int a; int b[1]; } *p;
int n = (0, p->b)[3];

... currently warns and trips the array-bounds sanitizer; with this change it would still warn but not trip the sanitizer, which seems bad. (Though I suppose the opposite case is worse.)