Page MenuHomePhabricator

[analyzer] A checker for dangling internal buffer pointers in C++
ClosedPublic

Authored by rnkovacs on May 21 2018, 4:47 AM.

Details

Summary

This check will mark raw pointers to C++ standard library container internal buffers "released" when the objects themselves are destroyed.
Such information can used by NewDeleteChecker to warn about use-after-free problems.

In this first version, std::basic_strings are supported.

Diff Detail

Repository
rL LLVM

Event Timeline

rnkovacs created this revision.May 21 2018, 4:47 AM
rnkovacs edited the summary of this revision. (Show Details)May 21 2018, 4:48 AM
MTC added a subscriber: MTC.May 21 2018, 5:51 AM
MTC added inline comments.May 21 2018, 6:28 AM
lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp
59 ↗(On Diff #147764)

A little tip, there are other string types besides std::string, like std::wstring, which can be added in the future. See basic_string.

71 ↗(On Diff #147764)

CXXDestructorCall::classof(const CallEvent *CA) can also be used here.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
3002 ↗(On Diff #147764)

In addition to std::string, std::vector::data() in C++11 and std::string_view in C++17 may have similar problems. If these are all supported, a new AllocationFamily may be needed, like AF_StdLibContainers or something like that.

lib/StaticAnalyzer/Checkers/RegionState.h
18 ↗(On Diff #147764)

I'm not sure whether region_state follows the naming conventions of LLVM coding standards. Can someone answer this question?

Adding a preliminary test file.

NoQ added a comment.May 21 2018, 11:44 AM

This looks great, i think we should make a single super simple mock test and commit this.

@MTC, i really appreciate your help!

lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp
59 ↗(On Diff #147764)

Yup, the current check should work in many cases, but it should be better to compare the class name to basic_string.

I'm also worried about various sorts of std::__1::strings (an inline namespace in libc++).

64 ↗(On Diff #147764)

It has long been planned to extend isCalled to C++ methods, i.e. something like this:

DanglingStringPointerChecker() : CStrFn("std::string::c_str") {}

...

void DanglingStringPointerChecker::checkPostCall(const CallEvent &Call,
                                                 CheckerContext &C) const {
  // Skip the class check.
  if (Call.isCalled(CStrFn)) {
    ...
  }

  ...
}

Still looking for volunteers^^

71 ↗(On Diff #147764)

isa.

lib/StaticAnalyzer/Checkers/RegionState.h
18 ↗(On Diff #147764)

I think it does, cf. namespace ast_matchers.

The name should be more specific though, a lot of checkers track "region states". Maybe "allocation_state"?

lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp
29 ↗(On Diff #147764)

"string" is a bit ambiguous, if this checker is specifically for std::string should we change the name to reflect that?

xbolva00 added inline comments.
lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp
29 ↗(On Diff #147764)

Agree, maybe DanglingStdStringPointerChecker?

Thanks for your comments!

It would be nice if we could reach a consensus on the naming issue before I update the patch. I was wondering, as we plan to support stuff like std::vector::data(), which is not a string, and std::string_view, which is not strictly a pointer, could we perhaps go with something like DanglingInternalBufferHandle? The AllocationFamily could similarly be AF_InternalBuffer. What do you think?

NoQ added a comment.May 22 2018, 1:23 PM

We'll have to track string_view ourselves, not relying on MallocChecker. So we only need an AF_ for the pointer case.

DanglingInternalBufferChecker and AF_InternalBuffer sound great to me.

rnkovacs updated this revision to Diff 148727.May 26 2018, 11:13 AM
rnkovacs marked 9 inline comments as done.
rnkovacs retitled this revision from [analyzer][WIP] A checker for dangling string pointers in C++ to [analyzer] A checker for dangling internal buffer pointers in C++.
rnkovacs edited the summary of this revision. (Show Details)
  • All basic_string types are now supported.
  • Mock tests added.
  • New AllocationFamily AF_InternalBuffer introduced.
  • NewDeleteChecker dependency added.

Looks good so far, some comments inline.

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
58 ↗(On Diff #148727)

QualType should have overloaded -> operator, I think you can remove the getTypePtr.

65 ↗(On Diff #148727)

I wonder if we can always get a symbol.
I can think of two cases when the call above could fail:

  • Non-standard implementation that does not return a pointer
  • The analyzer able to inline stuff and the returned value is a constant (a specific address that is shared between all empty strings in some implementation?)

Even though I do find any of the above likely. @NoQ what do you think? Does this worth a check?

73 ↗(On Diff #148727)

What if no symbol is associated with the region? Won't this return null that we dereference later on?

test/Analysis/dangling-internal-buffer.cpp
24 ↗(On Diff #148727)

I would like to see test cases that does not trigger warning.

xazax.hun added inline comments.May 26 2018, 11:30 AM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
73 ↗(On Diff #148727)

Oh, never mind this one, I did not notice the contains call above.

xazax.hun added inline comments.May 26 2018, 11:32 AM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
65 ↗(On Diff #148727)

Sorry for the spam. Unfortunately, it is not possible to edit the comments.
I do *not* find any of the above likely.

rnkovacs updated this revision to Diff 148732.May 26 2018, 11:50 AM
rnkovacs marked 4 inline comments as done.

Address (most) comments.

This revision is now accepted and ready to land.May 26 2018, 11:55 AM
rnkovacs added inline comments.May 26 2018, 12:03 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1661 ↗(On Diff #148732)

Is tying this new family to NewDeleteChecker reasonable? I did it because it was NewDelete that warned naturally before creating the new AllocationFamily. Should I introduce a new checker kind in MallocChecker for this purpose instead?

xbolva00 added inline comments.May 26 2018, 2:06 PM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
33 ↗(On Diff #148732)

string.data() support?

NoQ accepted this revision.May 27 2018, 11:05 PM

Looks great, thanks! I think you should add a check for UnknownVal and commit.

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
65 ↗(On Diff #148727)

We should almost always check if any value is an UnknownVal. The engine simply doesn't give any guarantees: it can give up any time and fall back to UnknownVal.

73 ↗(On Diff #148727)

The interesting part here is what happens if no expression is associated with the call. For instance, if the call is an automatic destructor at the end of scope. I hope it works, but i'm not sure how Origin is used.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1661 ↗(On Diff #148732)

Uhm, this code is weird.

I think you can add an "external" ("associated", "plugin") CheckKind that always warns.

rnkovacs updated this revision to Diff 148827.May 28 2018, 10:18 AM

Added a check for UnknownVal and two FIXMEs (one for the OriginExpr and one for the new CheckKind).

NoQ added inline comments.May 30 2018, 11:44 AM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
33 ↗(On Diff #148732)

Yup, there's a lot of API support improvements planned for later reviews.

xbolva00 accepted this revision.Jun 1 2018, 3:03 PM

Looks fine, awesome feature!

Closed by commit rL334348: [analyzer] Add dangling internal buffer check. (authored by rkovacs, committed by ). · Explain WhyJun 9 2018, 6:08 AM
This revision was automatically updated to reflect the committed changes.