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

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

Szelethus accepted this revision.
Szelethus marked 2 inline comments as done.
Szelethus added a comment.

LGTM! Mind that I just published D76917 <https://reviews.llvm.org/D76917>, you can, if you prefer, rebase on top of that. Also, a test case for `delete`ing a `ZERO_SIZE_PTR` valued pointer might be nice :)

If you prefer, feel free to commit without another round of reviews.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:396
+  /// yes was the value obtained or not.
+  mutable Optional<Optional<int>> KernelZeroSizePtrValue;
+
----------------
balazske wrote:
> martong wrote:
> > balazske wrote:
> > > martong wrote:
> > > > Which one is referred to the lazy initialization? The inner or the outer?
> > > > These questions actually made me to come up with a more explanatory construct here:
> > > > Could we do something like this?
> > > > ```
> > > > using LazyInitialized = Optional<int>;
> > > > mutable Optional<LazyInitialized> KernelZeroSizePtrValue; // Or Lazy<Optional<...>>
> > > > ```
> > > Probably use a `std::unique_ptr<Optional<int>>` instead (like at the bug types, they could be optional too)?
> > > If there is a code like
> > > ```
> > > bool IsSomethingInitialized;
> > > int Something;
> > > ```
> > > that looks as a clear case to use an optional (or unique_ptr)? And if yes, is a reason for not using this construct if `int` is replaced by `Optional<int>`?
> > Now I see that the lazy initialization is represented by the outer optional.
> > So IMHO a using template could be the best to describe cleanly this construct:
> > ```
> > template <class T>
> > using LazyInitialized = Optional<T>;
> > mutable LazyInitialized<Optional<int>> KernelZeroSizePtrValue;
> > ```
> > 
> With this I would wait for opinion of somebody else.
What @martong is saying is indeed nice, but the comments explain whats happening as well -- I'll leave it up to you how you want to commit this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1687
+  if (ArgValKnown) {
+    if (!KernelZeroSizePtrValue)
+      KernelZeroSizePtrValue =
----------------
balazske wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > Szelethus wrote:
> > > > > 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 :)
> > > > Just pushed rG4dc8472942ce60f29de9587b9451cc3d49df7abf. It it settles (no buildbots complain), you could rebase and the lazy initialization could be a thing of the past! :)
> > > This makes it possible to have a generic one-time initialization function in the checker (`CheckerBase`)? The functionality is needed in more than one checker, at least for the bug types it can be used in almost every checker (that has bugtypes).
> > I'm not sure I understand what you mean -- do you want to add `CheckerManager` to `CheckerBase`'s constructor?
> > 
> > In any case, I remember reading something around `CheckerBase`'s or `BugType`'s implementation the way the checker receives its name is a bit tricky and not a part of the construction.
> Initialization in register function does not work: The `MacroInfo` is not found in the preprocessor. Probably the code is not parsed normally at that time. For now only the lazy initialization is working.
> Initialization in register function does not work: The MacroInfo is not found in the preprocessor. Probably the code is not parsed normally at that time. For now only the lazy initialization is working.

Well that is super annoying. I don't object to the lazy initialization then.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1692-1694
+    // 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.
----------------
Just a bit of word smithing :)

If the macro ZERO_SIZE_PTR is defined, this could be a kernel source code. In that case, the ZERO_SIZE_PTR defines indicates a special value used for a zero-sized memory block which is allowed to be freed, despite not being a null pointer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76830