Page MenuHomePhabricator

[Analyzer] Hotfix for various crashes in iterator checkers
ClosedPublic

Authored by baloghadamsoftware on Jul 7 2020, 4:45 AM.

Details

Summary

The patch that introduces handling iterators implemented as pointers may cause crash in some projects because pointer difference is mistakenly handled as pointer decrement. (Similair case for iterators implemented as class instances are already handled correctly.) This patch fixes this issue.

The second case that causes crash is comparison of an iterator implemented as pointer and a null-pointer. This patch contains a fix for this issue as well.

The third case which causes crash is that the checker mistakenly considers all integers as nonloc::ConcreteInt when handling an increment or decrement of an iterator implemented as pointers. This patch adds a fix for this too.

The last case where crashes were detected is when checking for success of an std::advance() operation. Since the modeling of iterators implemented as pointers is still incomplete this may result in an assertion. This patch replaces the assertion with an early exit and adds a FIXME there.

Diff Detail

Event Timeline

baloghadamsoftware edited the summary of this revision. (Show Details)

Test added for the third fix in this patch.

Szelethus added inline comments.Jul 9 2020, 6:22 AM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
275–276

This doesn't look symmetrical. How does this patch interact with D83190?

baloghadamsoftware marked an inline comment as done.Jul 9 2020, 7:21 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
275–276

This is not symmetrical. Since this patch is a hotfix for three bugs, all of them cause crashes it is more urgent to land. The other patch should be rebased on it.

gamesh411 accepted this revision.Jul 9 2020, 7:22 AM
gamesh411 added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
275–276

I see that patch D83190 will need not only to be rebased, but modified slightly to take this early checking into account. I will refer to this review over there.

This revision is now accepted and ready to land.Jul 9 2020, 7:22 AM

@Szelethus thanks for being watchful, appreciated c:

Szelethus accepted this revision.Jul 9 2020, 7:36 AM

LGTM, but if we knowingly a subpar solution, we should make that clear in the surrounding code. I know the followup patch is around the corner, but just to be sure.

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
275–276

Please add a FIXME before commiting, if we knowingly add a hack.

466–470

Is this always okay? Shouldn't we check what the reason was behind the failure to retrieve the position? Is a TODO: at the very least appropriate?

clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
172–173

FIXME:

baloghadamsoftware edited the summary of this revision. (Show Details)

Two more crashes detected, fixes for them added.

Protection against Unknown added for pointer increments and decrements. No more crashes on LLVM/Clang.

gamesh411 accepted this revision.Jul 16 2020, 6:09 AM
This revision was automatically updated to reflect the committed changes.