This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] IteratorRangeChecker verify `std::advance()`, `std::prev()` and `std::next()`
ClosedPublic

Authored by baloghadamsoftware on Mar 18 2020, 12:26 PM.

Details

Summary

Upon calling one of the functions std::advance(), std::prev() and std::next() iterators could get out of their valid range which leads to undefined behavior. If all these funcions are inlined together with the functions they call internally (e.g. __advance() called by std::advance() in some implementations) the error is detected by IteratorRangeChecker but the bug location is inside the STL implementation. Even worse, if the budget runs out and one of the calls is not inlined the bug remains undetected. This patch fixes this behavior: all the bugs are detected at the point of the STL function invocation.

Diff Detail

Event Timeline

This patch replaces the verification part of D62895.

Szelethus accepted this revision.Mar 18 2020, 12:56 PM

The patch looks great, though I'd kindly ask you to wait a bit for someone with a bit more experience on SVal-smithing ;)

Generally speaking, I think the word you are looking for is "validate", not "verify", is that right, @whisperity?

clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
49–50

Prefer using.

This revision is now accepted and ready to land.Mar 18 2020, 12:56 PM

Updated according to the comments and automatic formatting suggestions.

baloghadamsoftware marked an inline comment as done.Mar 19 2020, 3:23 AM

The patch looks great, though I'd kindly ask you to wait a bit for someone with a bit more experience on SVal-smithing ;)

Do you mean the change from const SVal & to SVal? It was done according to this comment.

Generally speaking, I think the word you are looking for is "validate", not "verify", is that right, @whisperity?

We always used the word "verify" in the iterator checkers for checking for bugs. The word "check" is reserved for the hooks, "handle" is used in the modeling. This is an internal concept for the container-iterator ecosystem, I do not know about common Clang Static Analyzer coding standard here.

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Mar 23 2020, 4:41 PM

The patch looks great, though I'd kindly ask you to wait a bit for someone with a bit more experience on SVal-smithing ;)

Do you mean the change from const SVal & to SVal? It was done according to this comment.

I think @Szelethus means my inline comment.

clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
133

Please think about the type of the integer. You most likely want SValBuilder::makeArrayIndex(). There's also SValBuilder::makeIntVal() with a bunch of handy overloads. You almost never need to access BasicValueFactory directly.