[PATCH] D76830: [Analyzer][MallocChecker] No warning for kfree of ZERO_SIZE_PTR.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D76830#1943133 <https://reviews.llvm.org/D76830#1943133>, @balazske wrote:

> FIXME: There is a test file "kmalloc-linux.c" but it seems to be non-maintained and buggy (has no //-verify// option so it passes always but produces multiple warnings).


Crap, even I did some changes on that file, and never noticed the lack of verify, or the lack of `-analyzer-checker` flags.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1687
+  if (ArgValKnown) {
+    if (!KernelZeroSizePtrValue)
+      KernelZeroSizePtrValue =
----------------
martong wrote:
> balazske wrote:
> > martong wrote:
> > > martong wrote:
> > > > This is a bit confusing for me. Perhaps alternatively we could have a free function `isInitialized(KernelZero...)` instead. Or maybe having a separate bool variable to indicate whether it was initialized could be cleaner?
> > > Another idea: Adding a helper struct to contain the bool `initialized`? E.g. (draft):
> > > ```
> > > struct LazyOptional {
> > >   bool initialized = false;
> > >   Opt<int> value;
> > >   Opt& get();
> > >   void set(const Opt&);
> > > };
> > > ```
> > It may be OK to have a function `lazyInitKernelZeroSizePtrValue`?
> I don't insist, given we have a better described type for `KernelZeroSizePtrValue` (e.g. having a `using` `template`)
`AnalysisManager` has access to the `Preprocessor`, and it is also responsible for the construction of the `CheckerManager` object. This would make moving such code to the checker registry function! I'll gladly take this issue off your hand and patch it in once rG2aac0c47aed8d1eff7ab6d858173c532b881d948 settles :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1690-1700
+  // If there is a macro called ZERO_SIZE_PTR, it could be a kernel source code
+  // and this value indicates a special value used for a zero-sized memory
+  // block. It is a constant value that is allowed to be freed.
+  const llvm::APSInt *ArgValKnown =
+      C.getSValBuilder().getKnownValue(State, ArgVal);
+  if (ArgValKnown) {
+    initKernelZeroSizePtrValue(C.getPreprocessor());
----------------
I found this article on the subject:

https://lwn.net/Articles/236920/

> That brings us to the third possibility: this patch from Christoph Lameter which causes kmalloc(0) to return a special ZERO_SIZE_PTR value. It is a non-NULL value which looks like a legitimate pointer, but which causes a fault on any attempt at dereferencing it. Any attempt to call kfree() with this special value will do the right thing, of course.

But I would argue that using `delete` on such a pointer might still be a sign of code smell. Granted, if the source code is hacking the kernel this is very unlikely, but still, I think this code should be placed...


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1707-1708
   if (!R) {
     ReportBadFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr, Family);
     return nullptr;
   }
----------------
...here!

```lang=cpp
bool isArgZERO_SIZE_PTR(ArgVal) const {
  const llvm::APSInt *ArgValKnown =
      C.getSValBuilder().getKnownValue(State, ArgVal);
  if (ArgValKnown) {
    initKernelZeroSizePtrValue(C.getPreprocessor());
    if (*KernelZeroSizePtrValue &&
        ArgValKnown->getSExtValue() == **KernelZeroSizePtrValue)
      return true;
  }
  return false;
}

// ...

if (ArgVal has no associated MemRegion)
  // If there is a macro called ZERO_SIZE_PTR, it could be a kernel source code
  // and this value indicates a special value used for a zero-sized memory
  // block. It is a constant value that is allowed to be freed.
  if (!isArgZERO_SIZE_PTR(ArgVal)
    ReportBadFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr, Family);
    return nullptr;
  }
  // Still check for incorrect deallocator usage, etc.
}
```

Or something like this. WDYT?


================
Comment at: clang/test/Analysis/kmalloc-linux-1.c:15
+
+// FIXME: malloc checker expects kfree with 2 arguments, is this correct?
+// (recent kernel source code contains a `kfree(const void *)`)
----------------
martong wrote:
> Do you plan to sniff around a bit about the arguments (as part of another patch)? Would be good to handle both old and new signature kernel allocation functions.
I'll take a quick look as well, I am quite familiar with MallocChecker.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76830/new/

https://reviews.llvm.org/D76830