[PATCH] D154838: [analyzer] Add check for null pointer passed to the %p of printf family

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

I'd suggest renaming the checker to `alpha.unix.NullFormatSpecifier`.
Maybe add tests where multiple format specifiers are null, and also one test where no variadic args are present.

I also tried to simplify your solution a bit, so that we don't need to specify manually which arguments are for the "variadic" part.
Check it out here: F28923128: simplified.patch <https://reviews.llvm.org/F28923128>

We should think about a shorter diagnostic message because it seems unnecessarily long to me.
The rest looks good to me.

I could do a measurement to see how this would behave in the wild.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIPortabilityMinorChecker.cpp:70-71
+      new BugType(this,
+                  "Passing a null pointer to the pointer conversion "
+                  "specifier of ",
+                  categories::UnixAPI));
----------------
Is this an unfinished sentence? I guess you could remove the ` of ` suffix.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIPortabilityMinorChecker.cpp:86
+  ExplodedNode *N =
+      C.generateNonFatalErrorNode(nullState ? nullState : C.getState());
+  if (!N)
----------------
You always pass this state.


================
Comment at: clang/test/Analysis/unix-api-portability-minor.cpp:5-13
+struct FILE_t;
+typedef struct FILE_t FILE;
+
+typedef __typeof(sizeof(int)) size_t;
+
+int printf(const char *, ... );
+int fprintf(FILE *, const char *, ...);
----------------



================
Comment at: clang/test/Analysis/unix-api-portability-minor.cpp:30-35
+  printf(format, NULL); // expected-warning{{The result of passing a null pointer to the pointer conversion specifier of the printf family of functions is implementation defined}}
+  printf(format, 1, NULL); // expected-warning{{The result of passing a null pointer to the pointer conversion specifier of the printf family of functions is implementation defined}}
+  printf(format, 1, NULL, 2); // expected-warning{{The result of passing a null pointer to the pointer conversion specifier of the printf family of functions is implementation defined}}
+  printf(format, NULL, NULL); // expected-warning{{The result of passing a null pointer to the pointer conversion specifier of the printf family of functions is implementation defined}}
+  printf(format, NULL, 1, NULL); // expected-warning{{The result of passing a null pointer to the pointer conversion specifier of the printf family of functions is implementation defined}}
+  printf(format, 0); // no-warning
----------------
This format makes the pattern more apparent.


================
Comment at: clang/test/Analysis/unix-api-portability-minor.cpp:42-43
+  printf(format, pointer1); // no-warning
+  // Pointer argument should not get constrained after the check.
+  *pointer1 = 777; // no-warning
+
----------------
If `printf` had assumed that `pointer1` must be non-null to make the postcondition of the function pass, the dereference would succeed afterward anyway.

To test if `printf` didn't assume this, just have this afterward to make this observable:
```lang=C++
if (pointer1) {
  *pointer1 = 777;
} else {
  // We should have a warning here, as 'printf' didn't assume that the specifier is non-null.
  *pointer1 = 888; // expect-warning {{deref}}
}
```
If the pointer was constrained, then the `else` branch should be proven dead-code, and we should not have a warning


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154838