This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Fix assertions in commit r246345 (pr22954).
ClosedPublic

Authored by pgousseau on Sep 2 2015, 12:08 PM.

Details

Summary

Fix assertions kindly reported by Gabor Horvath and Piotr Zegar caused by commit r246345 and reverted in r246479.
Original review in http://reviews.llvm.org/D11832
Issue is caused by 'IsFirstBufInBound(...)' I wrongly assumed that no 'Unknown' values could reach it.
Added tests for unknown arguments being passed to 'memcpy' call.

Please let me know if this is an acceptable change ?

Regards,

Pierre Gousseau
SN-Systems - Sony Computer Entertainment

Diff Detail

Repository
rL LLVM

Event Timeline

pgousseau updated this revision to Diff 33838.Sep 2 2015, 12:08 PM
pgousseau retitled this revision from to [Analyzer] Fix assertions in commit r246345 (pr22954)..
pgousseau updated this object.
pgousseau added a subscriber: cfe-commits.
dcoughlin added inline comments.Sep 8 2015, 2:16 PM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
843 ↗(On Diff #33838)

There doesn't seem to be a test that cares about this returning true (as compared to false).

855 ↗(On Diff #33838)

There doesn't appear to be a test that cares about this returning true (as compared to false).

863 ↗(On Diff #33838)

What's the rationale for treating the array access as in-bounds if the BufEnd is unknown? Or if LengthValue is unknown? Should these branches return false? Either way, can you add a comment explaining why this is the right thing to do and also update the doc comment of IsFirstBufInBound to reflect this behavior (e.g., "Returns true if destination buffer of copy function must/may be in bound")

lib/StaticAnalyzer/Core/RegionStore.cpp
1109 ↗(On Diff #33838)

This assertion is triggering on our internal build bots. I'm working to get a reduced test case.

test/Analysis/pr22954.c
739 ↗(On Diff #33838)

A question: how does this test tainted values?

pgousseau added inline comments.Sep 9 2015, 12:26 PM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
843 ↗(On Diff #33838)

Will add thanks, f263 was the test meant for it.

855 ↗(On Diff #33838)

Will add thanks, f34 was the test meant for it.

863 ↗(On Diff #33838)

Yes I mean to return true in the case of an unknown array access to not warn on unknown state, will add comment thanks.

lib/StaticAnalyzer/Core/RegionStore.cpp
1109 ↗(On Diff #33838)

I can replace the assert by an if statement meanwhile ?

test/Analysis/pr22954.c
739 ↗(On Diff #33838)

Yes this comment is wrong, this is not testing tainted values. Will change comment and description thanks.

pgousseau updated this revision to Diff 34359.Sep 9 2015, 12:36 PM
pgousseau updated this object.

Following Devin's review:
Correct test for unknown length and unknown destination buffer.
Add comment to 'IsFirstBufInBound' behavior regarding unknown states.
Remove comments regarding tainted values.
Replace assertion regarding negative Region offset by an if statement.

Thanks for reviewing !

Pierre

dcoughlin added inline comments.Sep 17 2015, 6:41 PM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
198 ↗(On Diff #34359)

Since this returns true on unknown, it should be "may".

lib/StaticAnalyzer/Core/RegionStore.cpp
1109 ↗(On Diff #34359)

I think it is important to figure out enough of why the assert was triggering so that we can add a test case for this 'if'. The preprocessed file that is failing our bots is from the bugzilla for PR13765, in the clang2 attachment.

You can reproduce with:

clang -cc1 -analyze -analyzer-checker=core,cplusplus -fcxx-exceptions -std=c++11 clang2/clang_crash_4pFspw.ii
dcoughlin edited edge metadata.Sep 17 2015, 9:53 PM

Here is a reduced test case:

class B {
public:
   B &operator= (const B &v) {
      return *this;
   }
};

class A {
  int a[1];
  B b;
};

typedef long int ptrdiff_t;

void copyInto(A *first, A *last, A *result) {
  ptrdiff_t n;
  for (n = last - first; n > 0; --n)
     *--result = *--last;
}

Here is a reduced test case:

Very useful thanks !

lib/StaticAnalyzer/Checkers/CStringChecker.cpp
198 ↗(On Diff #34359)

Makes sense thanks !

lib/StaticAnalyzer/Core/RegionStore.cpp
1109 ↗(On Diff #34359)

Will have a look !

pgousseau updated this revision to Diff 35505.Sep 23 2015, 8:24 AM
pgousseau edited edge metadata.

Following Devin's review:

Fix a comment in 'IsFirstBufInBound'.
Remove incorrect assertion expecting 'RegionOffset::getOffset' to only return positives values.
Add test cases for regions having negatives offsets.

Updated a 'RegionOffset' class comment which seemed to suggest that 'RegionOffset::getOffset' could not return negative values.
Added test cases and handling for arrays' regions whose upper boundary can overflow.

Let me know if this is an acceptable change ?

Regards,

Pierre Gousseau

dcoughlin accepted this revision.Sep 23 2015, 6:25 PM
dcoughlin edited edge metadata.

Looks good to me! Thanks Pierre! I will commit.

This revision is now accepted and ready to land.Sep 23 2015, 6:25 PM

Looks good to me! Thanks Pierre! I will commit.

Much appreciated thanks !

This revision was automatically updated to reflect the committed changes.