[analyzer][MallocChecker] PR46253: Correctly recognize standard realloc

[Szelethus]

Summary

https://bugs.llvm.org/show_bug.cgi?id=46253

This is an obvious hack because realloc isn't any more affected than other functions modeled by MallocChecker (or any user of CallDescription really), but the nice solution will take some time to implement.

[NoQ]

Thanks! Yeah, that's a lot of annoying code to write that doesn't need to be imperative at all.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1074–1075

https://bugs.llvm.org/show_bug.cgi?id=46253#c1
Uh-oh, did we lose some sanity checking during CallDescription conversion?

While modeling of all functions is probably incorrect, the crash can be bisected down to D68165. I think other functions don't crash because they already have similar type checks in them, just more spread out around the code rather than concentrated in one place. It might still be worth it to try to figure out why exactly did D68165 cause it in order to double-check for more regressions.

[martong]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1071–1072

Indeed. CallDescrtiption could be improved to do precise type checking. Also it could be matching FunctionDecls instead of names of functions (strings), we see how error prone this can be.

I think, the mechanisms in StdCLibraryFunctionChecker could be integrated into CallDescription, so all other checkers could benefit.

See also the discussion we had on this with @xazax.hun :

In D80016#2063978, @martong wrote:

In D80016#2063234, @xazax.hun wrote:

A high level comment.

Trying to match function signatures is not a unique problem! In fact, almost all of the checks the analyzer have is trying to solve the very some problem.
One of the methods we have at this point is called CallDescription, see here: https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Core/CallEvent.cpp#L358

Moreover, I would assume something similar needs to be done for APINotes.

Do you think it would be possible to extend the CallDescription interface to match your needs? In that case all of the checks could profit from this work.
What do you think?

Yes, viewing from this angle, I am trying to solve a problem here that perhaps could be handled by CallDescription and CallDescriptionMap with some extensions.

Here are the differences between the two approaches so far:

• CallDescription is evaluated for every call event (e.g. checkPreCall), the names - strings - are compared; containing declaration contexts are compared by names (see CallEvent::isCalled). Contrary to this, summaries are associated directly to FunctionDecls, so during a call event we compare two FunctionDecl pointers.
• A CallDescription does not support overloading on different types if the number of parameters are the same. With summaries this works.
• A CallDescriptionMap is a static map with fixed number of entries. Contrary to this, the FunctionSummaryMap contains an entry (a summary) for a function only if we can lookup a matching FunctionDecl in the TU. The initialization of the FunctionSummaryMap happens lazily during the first call event.

Except the lack of proper overloading support, these differences are in the implementation. So, yes, giving it more thought, maybe we could refactor CallDescriptionMap to behave this way, but that would be some very heavy refactoring :) Still, would be possible, I guess.

Moreover, I would assume something similar needs to be done for APINotes.

Yes, I agree, I could not find out (yet) how the do it, but somehow they must match a given FunctionDecl to the Name in the YAML.

---

I was thinking about an alternative method for a long time now.
Making one step backwards: what we need is to be able to associate some data to a given function. And we can perfectly identify the function with it's prototype written in C/C++.
So, what if we'd be able to write a CallDescriptionMap (or a FunctionSummaryMap) like this:

CallDescriptionMap<FnCheck> Callbacks = {
    {{"void *memcpy(void *dest, const void *src, size_t n)"}, &CStringChecker::evalMemcpy},
    ...

Actually, we could reuse the Parser and the Sema itself to solve this issue. In fact, we could achieve this by reusing the ExternalASTSource together with the ASTImporter to find precisely the existing redecl chain in the TU for memcpy.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 //  the above checkers, it has it's own file, hence the many InnerPointerChecker
 //  related headers and non-static functions.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AllocationState.h"
 #include "InterCheckerAPI.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
void MallocChecker::checkKernelMalloc(const CallEvent &Call,
   if (MaybeState.hasValue())
     State = MaybeState.getValue();
   else
     State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State,
                          AF_Malloc);
   C.addTransition(State);
 }
 
-void MallocChecker::checkRealloc(const CallEvent &Call,
-                                 CheckerContext &C,
+static bool isStandardRealloc(const CallEvent &Call) {
+  const FunctionDecl *FD = dyn_cast<FunctionDecl>(Call.getDecl());
+  assert(FD);
+  ASTContext &AC = FD->getASTContext();
+
+  if (isa<CXXMethodDecl>(FD))
+    return false;
+
+  return FD->getDeclaredReturnType().getDesugaredType(AC) == AC.VoidPtrTy &&
+         FD->getParamDecl(0)->getType().getDesugaredType(AC) == AC.VoidPtrTy &&
+         FD->getParamDecl(1)->getType().getDesugaredType(AC) ==
+             AC.getSizeType();
+}
+
+static bool isGRealloc(const CallEvent &Call) {
+  const FunctionDecl *FD = dyn_cast<FunctionDecl>(Call.getDecl());
+  assert(FD);
+  ASTContext &AC = FD->getASTContext();
+
+  if (isa<CXXMethodDecl>(FD))
+    return false;
+
+  return FD->getDeclaredReturnType().getDesugaredType(AC) == AC.VoidPtrTy &&
+         FD->getParamDecl(0)->getType().getDesugaredType(AC) == AC.VoidPtrTy &&
+         FD->getParamDecl(1)->getType().getDesugaredType(AC) ==
+             AC.UnsignedLongTy;
+}
+
+void MallocChecker::checkRealloc(const CallEvent &Call, CheckerContext &C,
                                  bool ShouldFreeOnFail) const {
+  // HACK: CallDescription currently recognizes non-standard realloc functions
+  // as standard because it doesn't check the type, or wether its a non-method
+  // function. This should be solved by making CallDescription smarter.
+  // Mind that this came from a bug report, and all other functions suffer from
+  // this.
+  // https://bugs.llvm.org/show_bug.cgi?id=46253
+  if (!isStandardRealloc(Call) && !isGRealloc(Call))
+    return;
   ProgramStateRef State = C.getState();
   State = ReallocMemAux(C, Call, ShouldFreeOnFail, State, AF_Malloc);
   State = ProcessZeroAllocCheck(Call, 1, State);
   C.addTransition(State);
 }
 
 void MallocChecker::checkCalloc(const CallEvent &Call,
                                 CheckerContext &C) const {

clang/test/Analysis/malloc.cpp

 
 #define ZERO_SIZE_PTR ((void *)16)
 
 void test_delete_ZERO_SIZE_PTR() {
   int *Ptr = (int *)ZERO_SIZE_PTR;
   // ZERO_SIZE_PTR is specially handled but only for malloc family
   delete Ptr; // expected-warning{{Argument to 'delete' is a constant address (16)}}
 }
+
+namespace pr46253_class {
+class a {
+  void *realloc(int, bool = false) { realloc(1); } // no-crash
+};
+} // namespace pr46253_class
+
+namespace pr46253_retty{
+void realloc(void *ptr, size_t size) { realloc(ptr, size); } // no-crash
+} // namespace pr46253_retty
+
+namespace pr46253_paramty{
+void *realloc(void **ptr, size_t size) { realloc(ptr, size); } // no-crash
+} // namespace pr46253_paramty
+
+namespace pr46253_paramty2{
+void *realloc(void *ptr, int size) { realloc(ptr, size); } // no-crash
+} // namespace pr46253_paramty2