[analyzer] Fix false positives in inner pointer checker (PR49628)

[vsavchenko]

Summary

This patch supports std::data and std::addressof functions.

rdar://73463300

[steakhal]

I recommend splitting this into two. I would happily accept the part about std::data().

IMO std::addressof() should be modelled via evalCall as a pure function, instead of hacking it into the InnerPtrChecker.
It is overloaded to accept const T& as well, so the comment about "std::addressof function accepts a non-const reference as an argument" is not entirely true.

[vsavchenko]

In D99260#2650201, @steakhal wrote:

I recommend splitting this into two. I would happily accept the part about std::data().

IMO std::addressof() should be modelled via evalCall as a pure function, instead of hacking it into the InnerPtrChecker.

It does make sense to split these in two, but I'm not so sure about evalCall. It is definitely not something that InnerPtrChecker should be bothered with. Another point is that std::addressof might be only one of the function receiving a non-const reference and not modifying it, so it would
be nice to have a place in this checker where such exception is made.

It is overloaded to accept const T& as well, so the comment about "std::addressof function accepts a non-const reference as an argument" is not entirely true.

But it has a T & overload as well, and this is the one causing the FP.

[martong]

clang/test/Analysis/inner-pointer.cpp

23

Seems like all test are exercising with std::string, this looks like a legacy in this Checker.
Still, I miss a bit at least one test for the other overloads of std::data, maybe in a follow up patch?

378–392

So these are the FP cases that you are trying to solve?
Would be nice to see more details about the bug report (rdar://73463300) if that is not proprietary.

[vsavchenko]

clang/test/Analysis/inner-pointer.cpp

23

I can add it here, but what other test you suggest to add?

378–392

std::optional<std::string> str = "example";
char* dup = strndup(str->c_str(), str->size());

std::optional::operator-> uses std::addressof and the analyzer thinks that the pointer might get changed and raises the alarm.
I decided not to replicate std::optional in tests, and get straight to the point.

[martong]

clang/test/Analysis/inner-pointer.cpp

23

For example,

char a[20];
auto c = std::data(s);
consume(c);

Would this produce a warning?

Similarly to initializer list:

template <class E> 
constexpr const E* data(std::initializer_list<E> il) noexcept
{
    return il.begin();
}

int test() {
    auto IL = {0,1,2};
    const auto I = data(IL);
    consume(I);
    return 0;
}

378–392

Okay, thanks, makes sense.

[NoQ]

It does make sense to split these in two, but I'm not so sure about evalCall.

evalCall should work great for this purpose but definitely not in this checker. Also this checker would still exercise its checkPostCall so it would still need to be silenced even if evalCall is implemented.

clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

234

This will crash if someone overloads std::data with 0 arguments because your CallDescriptions don't require a specific number of arguments. (They're not allowed to do that, yes, but we're still not allowed to crash.)

[vsavchenko]

Require std::data to accept exactly 1 argument

[NoQ]

Looks great now, thanks!

[steakhal]

I'm still not satisfied with the addressof, but I won't block this either.

[NoQ]

In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that there are still problems with addressof (yes, their "trunk" clang is fresh enough). They seem to have __addressof instead of addressof so maybe we should cover that case real quick. Or maybe outright suppress reference invalidation on double-underscore functions because the checker generally relies on the number of standard functions that don't invalidate references while taking a non-const reference being very limited but this limit definitely doesn't take double-underscore functions into account.

[steakhal]

In D99260#2704102, @NoQ wrote:

In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that there are still problems with addressof (yes, their "trunk" clang is fresh enough). They seem to have __addressof instead of addressof so maybe we should cover that case real quick. Or maybe outright suppress reference invalidation on double-underscore functions because the checker generally relies on the number of standard functions that don't invalidate references while taking a non-const reference being very limited but this limit definitely doesn't take double-underscore functions into account.

What is the status quo of this issue? @vsavchenko

[vsavchenko]

In D99260#2751967, @steakhal wrote:

In D99260#2704102, @NoQ wrote:

In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that there are still problems with addressof (yes, their "trunk" clang is fresh enough). They seem to have __addressof instead of addressof so maybe we should cover that case real quick. Or maybe outright suppress reference invalidation on double-underscore functions because the checker generally relies on the number of standard functions that don't invalidate references while taking a non-const reference being very limited but this limit definitely doesn't take double-underscore functions into account.

What is the status quo of this issue? @vsavchenko

Ah, yep. Just got back from my vacation!
I'm not sure about shooting off double underscore functions because one never knows what weird coding conventions people have in their project (all TU-local static functions should be named like this for better visibility at their call sites, for example). Being more specific and dealing with std::__ can be better, but I think a quick hack specifically for std::__addressof is better atm.

[alishuja]

Based on the comment from @vsavchenko , I had made an addition for a check for __addressof alongside addressof in the checker. What would be the correct way of pushing the diff here? Should I reopen this revision or use the update diff button on the top? I apologize, I'm completely new to pushing changes to LLVM.

Thanks,

clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

 REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
 
 
 namespace {
 
 class InnerPointerChecker
     : public Checker<check::DeadSymbols, check::PostCall> {
 
-  CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
-      InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
-      ShrinkToFitFn, SwapFn;
+  CallDescription AppendFn, AssignFn, AddressofFn, ClearFn, CStrFn, DataFn,
+      DataMemberFn, EraseFn, InsertFn, PopBackFn, PushBackFn, ReplaceFn,
+      ReserveFn, ResizeFn, ShrinkToFitFn, SwapFn;
 
 public:
   class InnerPointerBRVisitor : public BugReporterVisitor {
     SymbolRef PtrToBuf;
 
   public:
     InnerPointerBRVisitor(SymbolRef Sym) : PtrToBuf(Sym) {}
 
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
       }
       return false;
     }
   };
 
   InnerPointerChecker()
       : AppendFn({"std", "basic_string", "append"}),
         AssignFn({"std", "basic_string", "assign"}),
+        AddressofFn({"std", "addressof"}),
         ClearFn({"std", "basic_string", "clear"}),
-        CStrFn({"std", "basic_string", "c_str"}),
-        DataFn({"std", "basic_string", "data"}),
+        CStrFn({"std", "basic_string", "c_str"}), DataFn({"std", "data"}, 1),
+        DataMemberFn({"std", "basic_string", "data"}),
         EraseFn({"std", "basic_string", "erase"}),
         InsertFn({"std", "basic_string", "insert"}),
         PopBackFn({"std", "basic_string", "pop_back"}),
         PushBackFn({"std", "basic_string", "push_back"}),
         ReplaceFn({"std", "basic_string", "replace"}),
         ReserveFn({"std", "basic_string", "reserve"}),
         ResizeFn({"std", "basic_string", "resize"}),
         ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
         SwapFn({"std", "basic_string", "swap"}) {}
 
   /// Check whether the called member function potentially invalidates
   /// pointers referring to the container object's inner buffer.
   bool isInvalidatingMemberFunction(const CallEvent &Call) const;
 
+  /// Check whether the called function returns a raw inner pointer.
+  bool isInnerPointerAccessFunction(const CallEvent &Call) const;
+
   /// Mark pointer symbols associated with the given memory region released
   /// in the program state.
   void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef State,
                               const MemRegion *ObjRegion,
                               CheckerContext &C) const;
 
   /// Standard library functions that take a non-const `basic_string` argument by
   /// reference may invalidate its inner pointers. Check for these cases and
return (isa<CXXDestructorCall>(Call) || Call.isCalled(AppendFn) ||
           Call.isCalled(AssignFn) || Call.isCalled(ClearFn) ||
           Call.isCalled(EraseFn) || Call.isCalled(InsertFn) ||
           Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) ||
           Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) ||
           Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) ||
           Call.isCalled(SwapFn));
 }
 
+bool InnerPointerChecker::isInnerPointerAccessFunction(
+    const CallEvent &Call) const {
+  return (Call.isCalled(CStrFn) || Call.isCalled(DataFn) ||
+          Call.isCalled(DataMemberFn));
+}
+
 void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent &Call,
                                                  ProgramStateRef State,
                                                  const MemRegion *MR,
                                                  CheckerContext &C) const {
   if (const PtrSet *PS = State->get<RawPtrMap>(MR)) {
     const Expr *Origin = Call.getOriginExpr();
     for (const auto Symbol : *PS) {
       // NOTE: `Origin` may be null, and will be stored so in the symbol's
for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
       unsigned ArgI = isaMemberOpCall ? I+1 : I;
 
       SVal Arg = FC->getArgSVal(ArgI);
       const auto *ArgRegion =
           dyn_cast_or_null<TypedValueRegion>(Arg.getAsRegion());
       if (!ArgRegion)
         continue;
 
+      // std::addressof function accepts a non-const reference as an argument,
+      // but doesn't modify it.
+      if (Call.isCalled(AddressofFn))
+        continue;
+
       markPtrSymbolsReleased(Call, State, ArgRegion, C);
     }
   }
 }
 
 // [string.require]
 //
 // "References, pointers, and iterators referring to the elements of a
 // basic_string sequence may be invalidated by the following uses of that
 // basic_string object:
 //
 // -- As an argument to any standard library function taking a reference
 // to non-const basic_string as an argument. For example, as an argument to
 // non-member functions swap(), operator>>(), and getline(), or as an argument
 // to basic_string::swap().
 //
 // -- Calling non-const member functions, except operator[], at, front, back,
 // begin, rbegin, end, and rend."
 
 void InnerPointerChecker::checkPostCall(const CallEvent &Call,
                                         CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
-  if (const auto *ICall = dyn_cast<CXXInstanceCall>(&Call)) {
-    // TODO: Do we need these to be typed?
-    const auto *ObjRegion = dyn_cast_or_null<TypedValueRegion>(
+  const TypedValueRegion *ObjRegion = nullptr;
+
+  if (const auto *ICall = dyn_cast<CXXInstanceCall>(&Call)) {
+    ObjRegion = dyn_cast_or_null<TypedValueRegion>(
         ICall->getCXXThisVal().getAsRegion());
+
+    // Check [string.require] / second point.
+    if (isInvalidatingMemberFunction(Call)) {
+      markPtrSymbolsReleased(Call, State, ObjRegion, C);
+      return;
+    }
+  }
+
+  if (isInnerPointerAccessFunction(Call)) {
+
+    if (isa<SimpleFunctionCall>(Call)) {
+      // NOTE: As of now, we only have one free access function: std::data.
+      //       If we add more functions like this in the list, hardcoded
+      //       argument index should be changed.
+      ObjRegion =
+          dyn_cast_or_null<TypedValueRegion>(Call.getArgSVal(0).getAsRegion());
+    }
+
     if (!ObjRegion)
       return;
 
-    if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-      SVal RawPtr = Call.getReturnValue();
-      if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
-        // Start tracking this raw pointer by adding it to the set of symbols
-        // associated with this container object in the program state map.
 
-        PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
-        const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion);
-        PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
-        assert(C.wasInlined || !Set.contains(Sym));
-        Set = F.add(Set, Sym);
 
-        State = State->set<RawPtrMap>(ObjRegion, Set);
-        C.addTransition(State);
-      }
-      return;
-    }
 
-    // Check [string.require] / second point.
-    if (isInvalidatingMemberFunction(Call)) {
-      markPtrSymbolsReleased(Call, State, ObjRegion, C);
-      return;
-    }
-  }
 
   // Check [string.require] / first point.
   checkFunctionArguments(Call, State, C);
 }
 
 void InnerPointerChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                            CheckerContext &C) const {
   ProgramStateRef State = C.getState();

clang/test/Analysis/inner-pointer.cpp

 void func_const_ref(const T &a);
 
 template <typename T>
 void func_value(T a);
 
 string my_string = "default";
 void default_arg(int a = 42, string &b = my_string);
 
+template <class T>
+T *addressof(T &arg);
+
+char *data(std::string &c);
+
 } // 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_swap() {
   const char *c;
   std::string s1, s2;
   c = s1.data(); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
   s1.swap(s2);   // expected-note {{Inner buffer of 'std::string' reallocated by call to 'swap'}}
   consume(c);    // expected-warning {{Inner pointer of container used after re/deallocation}}
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void deref_after_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
+  s.push_back('c'); // expected-note {{Inner buffer of 'std::string' reallocated by call to 'push_back'}}
+  consume(c);       // expected-warning {{Inner pointer of container used after re/deallocation}}
+  // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
+}
+
 struct S {
   std::string s;
   const char *name() {
     return s.c_str(); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
                       // expected-note@-1 {{Pointer to inner buffer of 'std::string' obtained here}}
   }
   void clear() {
     s.clear(); // expected-note {{Inner buffer of 'std::string' reallocated by call to 'clear'}}
void func_default_arg() {
   const char *c;
   std::string s;
   c = s.c_str();     // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
   default_arg(3, s); // expected-note {{Inner buffer of 'std::string' reallocated by call to 'default_arg'}}
   consume(c);        // expected-warning {{Inner pointer of container used after re/deallocation}}
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}
+
+void func_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s);
+  consume(c); // no-warning
+}
+
 struct T {
   std::string to_string() { return s; }
+
 private:
   std::string s;
 };
 
 const char *escape_via_return_temp() {
   T x;
   return x.to_string().c_str(); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
   // expected-note@-1 {{Inner buffer of 'std::string' deallocated by call to destructor}}