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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
