[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

3006

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

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

65

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

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

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

[xazax.hun]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

73

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

[xazax.hun]

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

65

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

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

34

string.data() support?

[NoQ]

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

lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp

65

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

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

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

34

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

[xbolva00]

Looks fine, awesome feature!

[xbolva00]

Ping

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

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

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

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().getTypePtr()->getAsCXXRecordDecl();
+  if (TypeDecl->getName() != "basic_string")
+    return;
+
+  ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(CStrFn)) {
+    SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
+    State = State->set<RawPtrMap>(TypedR, RawPtr);
+    C.addTransition(State);
+    return;
+  }
+
+  if (isa<CXXDestructorCall>(ICall)) {
+    if (State->contains<RawPtrMap>(TypedR)) {
+      const SymbolRef *StrBufferPtr = State->get<RawPtrMap>(TypedR);
+      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>();
+}

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:
+  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();

test/Analysis/dangling-internal-buffer.cpp

+//RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.DanglingInternalBuffer %s -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}}
+}