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

[rnkovacs]

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.

[MTC]

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?

[rnkovacs]

Adding a preliminary test file.

tests.cpp6 KBDownload

[NoQ]

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"?

[george.karpenkov]

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]

lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp

29 ↗

(On Diff #147764)

Agree, maybe DanglingStdStringPointerChecker?

[rnkovacs]

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]

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]

• All basic_string types are now supported.
• Mock tests added.
• New AllocationFamily AF_InternalBuffer introduced.
• NewDeleteChecker dependency added.

[xazax.hun]

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]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

73 ↗

(On Diff #148727)

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

[xazax.hun]

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]

Address (most) comments.

[xazax.hun]

LGTM!

[rnkovacs]

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]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

33 ↗

(On Diff #148732)

string.data() support?

[NoQ]

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]

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

[NoQ]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

33 ↗

(On Diff #148732)

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

[xbolva00]

Looks fine, awesome feature!

[xbolva00]

Ping

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">,
   DescFile<"VirtualCallChecker.cpp">;
 
 } // end: "optin.cplusplus"
 
 let ParentPackage = CplusplusAlpha in {
 
+def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
+  HelpText<"Check for internal raw pointers of C++ standard library containers "
+           "used after deallocation">,
+  DescFile<"DanglingInternalBufferChecker.cpp">;
+
 def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
   HelpText<"Reports destructions of polymorphic objects with a non-virtual "
            "destructor in their base class">,
   DescFile<"DeleteWithNonVirtualDtorChecker.cpp">;
 
 def IteratorRangeChecker : Checker<"IteratorRange">,
   HelpText<"Check for iterators used outside their valid ranges">,
   DescFile<"IteratorChecker.cpp">;

cfe/trunk/lib/StaticAnalyzer/Checkers/AllocationState.h

+//===--- AllocationState.h ------------------------------------- *- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONSTATE_H
+#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONSTATE_H
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+
+namespace clang {
+namespace ento {
+
+namespace allocation_state {
+
+ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym,
+                             const Expr *Origin);
+
+} // end namespace allocation_state
+
+} // end namespace ento
+} // end namespace clang
+
+#endif

cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   CheckSecuritySyntaxOnly.cpp
   CheckSizeofPointer.cpp
   CheckerDocumentation.cpp
   ChrootChecker.cpp
   ClangCheckers.cpp
   CloneChecker.cpp
   ConversionChecker.cpp
   CXXSelfAssignmentChecker.cpp
+  DanglingInternalBufferChecker.cpp
   DeadStoresChecker.cpp
   DebugCheckers.cpp
   DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

+//=== DanglingInternalBufferChecker.cpp ---------------------------*- C++ -*--//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a check that marks a raw pointer to a C++ standard library
+// container's inner buffer released when the object is destroyed. This
+// information can be used by MallocChecker to detect use-after-free problems.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "AllocationState.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class DanglingInternalBufferChecker : public Checker<check::PostCall> {
+  CallDescription CStrFn;
+
+public:
+  DanglingInternalBufferChecker() : CStrFn("c_str") {}
+
+  /// Record the connection between the symbol returned by c_str() and the
+  /// corresponding string object region in the ProgramState. Mark the symbol
+  /// released if the string object is destroyed.
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+};
+
+} // end anonymous namespace
+
+// FIXME: c_str() may be called on a string object many times, so it should
+// have a list of symbols associated with it.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+
+void DanglingInternalBufferChecker::checkPostCall(const CallEvent &Call,
+                                                  CheckerContext &C) const {
+  const auto *ICall = dyn_cast<CXXInstanceCall>(&Call);
+  if (!ICall)
+    return;
+
+  SVal Obj = ICall->getCXXThisVal();
+  const auto *TypedR = dyn_cast_or_null<TypedValueRegion>(Obj.getAsRegion());
+  if (!TypedR)
+    return;
+
+  auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+  if (TypeDecl->getName() != "basic_string")
+    return;
+
+  ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(CStrFn)) {
+    SVal RawPtr = Call.getReturnValue();
+    if (!RawPtr.isUnknown()) {
+      State = State->set<RawPtrMap>(TypedR, RawPtr.getAsSymbol());
+      C.addTransition(State);
+    }
+    return;
+  }
+
+  if (isa<CXXDestructorCall>(ICall)) {
+    if (State->contains<RawPtrMap>(TypedR)) {
+      const SymbolRef *StrBufferPtr = State->get<RawPtrMap>(TypedR);
+      // FIXME: What if Origin is null?
+      const Expr *Origin = Call.getOriginExpr();
+      State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
+      C.addTransition(State);
+      return;
+    }
+  }
+}
+
+void ento::registerDanglingInternalBufferChecker(CheckerManager &Mgr) {
+  registerNewDeleteChecker(Mgr);
+  Mgr.registerChecker<DanglingInternalBufferChecker>();
+}

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "AllocationState.h"
 #include <climits>
 #include <utility>
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
 enum AllocationFamily {
   AF_None,
   AF_Malloc,
   AF_CXXNew,
   AF_CXXNewArray,
   AF_IfNameIndex,
-  AF_Alloca
+  AF_Alloca,
+  AF_InternalBuffer
 };
 
 class RefState {
   enum Kind { // Reference to allocated memory.
               Allocated,
               // Reference to zero-allocated memory.
               AllocatedOfSizeZero,
               // Reference to released/freed memory.
void MallocChecker::printExpectedAllocName(raw_ostream &os, CheckerContext &C,
                                            const Expr *E) const {
   AllocationFamily Family = getAllocationFamily(C, E);
 
   switch(Family) {
     case AF_Malloc: os << "malloc()"; return;
     case AF_CXXNew: os << "'new'"; return;
     case AF_CXXNewArray: os << "'new[]'"; return;
     case AF_IfNameIndex: os << "'if_nameindex()'"; return;
+    case AF_InternalBuffer: os << "container-specific allocator"; return;
     case AF_Alloca:
     case AF_None: llvm_unreachable("not a deallocation expression");
   }
 }
 
 void MallocChecker::printExpectedDeallocName(raw_ostream &os,
                                              AllocationFamily Family) const {
   switch(Family) {
     case AF_Malloc: os << "free()"; return;
     case AF_CXXNew: os << "'delete'"; return;
     case AF_CXXNewArray: os << "'delete[]'"; return;
     case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
+    case AF_InternalBuffer: os << "container-specific deallocator"; return;
     case AF_Alloca:
     case AF_None: llvm_unreachable("suspicious argument");
   }
 }
 
 ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           const Expr *ArgExpr,
                                           const Expr *ParentExpr,
MallocChecker::getCheckIfTracked(AllocationFamily Family,
   case AF_Alloca:
   case AF_IfNameIndex: {
     if (ChecksEnabled[CK_MallocChecker])
       return CK_MallocChecker;
 
     return Optional<MallocChecker::CheckKind>();
   }
   case AF_CXXNew:
-  case AF_CXXNewArray: {
+  case AF_CXXNewArray:
+  // FIXME: Add new CheckKind for AF_InternalBuffer.
+  case AF_InternalBuffer: {
     if (IsALeakCheck) {
       if (ChecksEnabled[CK_NewDeleteLeaksChecker])
         return CK_NewDeleteLeaksChecker;
     }
     else {
       if (ChecksEnabled[CK_NewDeleteChecker])
         return CK_NewDeleteChecker;
     }
for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) {
       I.getData().dump(Out);
       if (CheckKind.hasValue())
         Out << " (" << CheckNames[*CheckKind].getName() << ")";
       Out << NL;
     }
   }
 }
 
+namespace clang {
+namespace ento {
+namespace allocation_state {
+
+ProgramStateRef
+markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) {
+  AllocationFamily Family = AF_InternalBuffer;
+  return State->set<RegionState>(Sym, RefState::getReleased(Family, Origin));
+}
+
+} // end namespace allocation_state
+} // end namespace ento
+} // end namespace clang
+
 void ento::registerNewDeleteLeaksChecker(CheckerManager &mgr) {
   registerCStringCheckerBasic(mgr);
   MallocChecker *checker = mgr.registerChecker<MallocChecker>();
   checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption(
       "Optimistic", false, checker);
   checker->ChecksEnabled[MallocChecker::CK_NewDeleteLeaksChecker] = true;
   checker->CheckNames[MallocChecker::CK_NewDeleteLeaksChecker] =
       mgr.getCurrentCheckName();

cfe/trunk/test/Analysis/dangling-internal-buffer.cpp

+//RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.DanglingInternalBuffer %s -analyzer-output=text -verify
+
+namespace std {
+
+template< typename CharT >
+class basic_string {
+public:
+  ~basic_string();
+  const CharT *c_str();
+};
+
+typedef basic_string<char> string;
+typedef basic_string<wchar_t> wstring;
+typedef basic_string<char16_t> u16string;
+typedef basic_string<char32_t> u32string;
+
+} // end namespace std
+
+void consume(const char *) {}
+void consume(const wchar_t *) {}
+void consume(const char16_t *) {}
+void consume(const char32_t *) {}
+
+void deref_after_scope_char() {
+  const char *c;
+  {
+    std::string s;
+    c = s.c_str();
+  }
+  consume(c); // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void deref_after_scope_wchar_t() {
+  const wchar_t *w;
+  {
+    std::wstring ws;
+    w = ws.c_str();
+  }
+  consume(w); // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void deref_after_scope_char16_t() {
+  const char16_t *c16;
+  {
+    std::u16string s16;
+    c16 = s16.c_str();
+  }
+  consume(c16); // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void deref_after_scope_char32_t() {
+  const char32_t *c32;
+  {
+    std::u32string s32;
+    c32 = s32.c_str();
+  }
+  consume(c32); // expected-warning {{Use of memory after it is freed}}
+  // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void deref_after_scope_ok() {
+  const char *c;
+  std::string s;
+  {
+    c = s.c_str();
+  }
+  consume(c); // no-warning
+}