This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Highlight c_str() call in DanglingInternalBuffer checker
ClosedPublic

Authored by rnkovacs on Jun 23 2018, 12:04 PM.

Details

Summary

Add a bug visitor to DanglingInternalBuffer checker that places a note at the point where the dangling pointer was obtained.
The visitor is handed over to MallocChecker and attached to the report there.

Diff Detail

Event Timeline

rnkovacs created this revision.Jun 23 2018, 12:04 PM

Regarding the visitor:
Maybe rather than looking at the AST, we should check the states, when we started to track the returned symbol?

Using your current design you need to check for the AST twice. Once in the visitor and once in the check.

Also, I wonder if this always give you the right note. Consider the following example:

void deref_after_scope_char() {
  const char *c;
  {
    std::string s;
    c = s.c_str();
  }
  std::string s;
  const char *c2 = s.c_str();
  consume(c); 
}
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
161

Why not !=?

rnkovacs updated this revision to Diff 152604.Jun 23 2018, 1:24 PM
rnkovacs marked an inline comment as done.

Um, sorry, I totally forgot about that. Added your case to the tests.

Looks better, thanks!

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
65

Maybe early return here?

NoQ added inline comments.Jun 23 2018, 4:07 PM
lib/StaticAnalyzer/Checkers/AllocationState.h
23–25

I think we should start commenting this stuff up. Like, "This function provides an additional visitor that augments the bug report with information relevant to memory errors caused by misuse of AF_InternalBuffer symbols".

NoQ accepted this revision.Jun 23 2018, 4:11 PM

Looks good tho!

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
63–64

Interesting, so we don't have access to the region with which the symbol is associated, so we have to scan the whole map.

Probably we can scan the map only once (eg., in the visitor's consturctor if we also supply the program state) and then do a direct lookup by region?

Because it's a premature optimization, i'm in favor of a FIXME.

155

Maybe "pointer to dangling buffer".

This revision is now accepted and ready to land.Jun 23 2018, 4:11 PM
rnkovacs updated this revision to Diff 152616.Jun 24 2018, 10:03 AM
rnkovacs marked 4 inline comments as done.

Thanks! Addressed comments.

xazax.hun accepted this revision.Jun 24 2018, 10:08 AM
xazax.hun added inline comments.
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
41

I am fine with this as is, but I prefer self documenting code in general. Naming this variable PtrToBuf or something like that would conway the same information and render the comment redundant.

rnkovacs updated this revision to Diff 152627.Jun 25 2018, 12:14 AM
rnkovacs marked an inline comment as done.

Fixed variable name inside the visitor.
I also clang-formatted the file, sorry for any line number shifting.

rnkovacs updated this revision to Diff 152719.Jun 25 2018, 10:26 AM

Fixed the constness of c_str() in the test file.

This revision was automatically updated to reflect the committed changes.