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

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

steakhal added a comment.

Looks pretty good.



================
Comment at: clang/docs/analyzer/checkers.rst:704
+optin.portabilityMinor.UnixAPI
+"""""""""""""""""""""""""
+Finds non-severe implementation-defined behavior in UNIX/Posix functions.
----------------
This line should be just as long, as the line above.


================
Comment at: clang/docs/analyzer/checkers.rst:705
+"""""""""""""""""""""""""
+Finds non-severe implementation-defined behavior in UNIX/Posix functions.
+
----------------
Our docs aren't great, but we should have a brief description what the checker detects, basically here it would be "Find null pointers being passed to printf-like functions. Usually, passing null pointers to such functions is implementation defined, thus non-portable. Printf-like functions are: x, y, z."
Illustrate one example for diagnosing a case. Also illustrate one case similar to the bad one, that is fixed (basically ensure that the pointer is not null there).
And that's it.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:45-50
+// The PortabilityMinor package is for checkers that find non-severe portability
+// issues (see also the Portability package). Such checks may be unwanted for
+// developers who want to ignore minor portability issues, hence they are put in
+// a separate package.
+def PortabilityMinorOptIn : Package<"portabilityMinor">, ParentPackage<OptIn>;
+
----------------
I don't think we should create a separate package. A separate checker should be enough in `alpha.unix`, or `alpha.optin`, or just `optin`.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:1671
+
+def UnixAPIPortabilityMinorChecker : Checker<"UnixAPI">,
+  HelpText<"Finds non-severe implementation-defined behavior in UNIX/Posix functions">,
----------------
Usually, the user-facing checker name which is the `<"...">` part there, matches the name of the checker's class name. It helps for maintenance.
I would strongly suggest keeping this naming convention.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:1673
+  HelpText<"Finds non-severe implementation-defined behavior in UNIX/Posix functions">,
+  Documentation<NotDocumented>;
+
----------------
Docs are not really optional these days, but we can come back later once the checker is ironed out.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:88
 
+class UnixAPIPortabilityMinorChecker
+    : public Checker<check::PreStmt<CallExpr>> {
----------------
Usually, we put separate checkers into their own file.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:89
+class UnixAPIPortabilityMinorChecker
+    : public Checker<check::PreStmt<CallExpr>> {
+public:
----------------
Prefer using the `check::PreCall` callback over the `PreStmt`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:94
+private:
+  mutable std::unique_ptr<BugType> BT_printfPointerConversionSpecifierNULL;
+
----------------
Nowdays, if I'm not mistaken, we just inline initialize these eagerly.
That way you could also avoid marking it mutable.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:102
+  void
+  ReportPrintfPointerConversionSpecifierNULL(clang::ento::CheckerContext &C,
+                                             ProgramStateRef nullState,
----------------
Feel free to just call it `reportBug`.
Also, in the cpp file, usually we use the namespaces `clang,llvm,ento` already. You don't need to fully-qualify these names.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:593-594
+
+  if (FName == "printf")
+    CheckPrintfPointerConversionSpecifierNULL(C, CE, 1);
+
----------------
For matching names, we usually use `CallDescriptions` to do it for us.
If think you could use them while also checking if the Call is a `CallEvent::isGlobalCFunction()`. I suspect, you only wanna check those.
I think you should define a `CallDescriptionMap`, because you will also need your specific `data_arg_index`...
Look for uses of such maps and you will get inspired.


================
Comment at: clang/test/Analysis/unix-fns.c:81-92
+#ifndef NULL
+#define NULL ((void*) 0)
+#endif
+
+struct FILE_t;
+typedef struct FILE_t FILE;
+
----------------
I'd highly recommend not touching this file to avoid the bulk change in the expected plist output. Please just introduce a new test file.


================
Comment at: clang/test/Analysis/unix-fns.c:280-289
+// Test pointer constraint.
+void printf_pointer_conversion_specifier_null_pointer_constraint(char *format, void *pointer1) {
+  void *pointer2 = NULL;
+  printf(format, pointer2); // expected-warning{{The result of passing a null pointer to the pointer conversion specifier of the printf family of functions is implementation defined}}
+  if (pointer1 != NULL) {
+    printf(format, pointer1); // no-warning
+    return;
----------------
I would appreciate a test demonstrating that the pointer argument won't get constrained after the `printf` call. I think such case was already discussed.

We should also demonstrate that unknown pointers (which doesn't known to be neither null, nor non-null) won't raise issues.

In the checker-logic, we have the `data_args_index` for each `printf`-like function.
Can you demonstrate that each is handled as expected?

Also have a test when the formatstring itself is null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154838