[PATCH] D75063: [analyzer] StdLibraryFunctionsChecker: Add NotNull Arg Constraint

[Gabor Marton via Phabricator via cfe-commits]

martong marked 6 inline comments as done.
martong added inline comments.
Herald added a subscriber: ASDenysPetrov.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:175
+    using ValueConstraint::ValueConstraint;
+    bool CannotBeNull = true;
+
----------------
Szelethus wrote:
> What does this do? Is it ever used in the patch?
Yes, it is used. We use it in `apply` the value is passed to `assume`.
And in `negate` we flip the value.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:189
+    ValueConstraintPtr negate() const override {
+      NotNullConstraint tmp(*this);
+      tmp.CannotBeNull = !this->CannotBeNull;
----------------
balazske wrote:
> Is `Tmp` better name?
Absolutely, yes, I should avoid lower case variables, will rename.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:684
+        .Case({
+            ReturnValueCondition(LessThanOrEq, ArgNo(2)),
+        })
----------------
steakhal wrote:
> Two lines below you are using the `{0U}` initialization syntax, and here the simple constructor call syntax.
> Shouldn't we pick one?
Yes, definitely. I think I am going to use brace initialization syntax everywhere.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:59
+    fread(buf, sizeof(int), 10, fp); // expected-warning{{Function argument constraint is not satisfied}}
+}
----------------
balazske wrote:
> One difficulty is that `fread` can work if the `buf` is 0 and `count` is 0 too. There is for sure code that uses this feature.
My understanding is that in case of `fread`, if `buf` is 0 then that is an undefined behavior.

In the section in C11 describing the use of library functions (§7.1.4) it is stated that:



> If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined. If a function argument is described as being an array, the pointer actually passed to the function shall have a value such that all address computations and accesses to objects (that would be valid if the pointer did point to the first element of such an array) are in fact valid.

I've found more details in this StackOverflow post:
https://stackoverflow.com/questions/51779960/is-it-safe-to-fread-0-bytes-into-a-null-pointer

Also note that a sane libc implementation would not dereference `buf` if `size` is 0, but we may not rely on these implementation details, that is why this is declared as undefined behavior in the standard.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75063