[PATCH] D154838: [analyzer] Add check for null pointer passed to the %p of printf family
Balázs Benics via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 31 02:47:05 PDT 2023
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
More information about the cfe-commits
mailing list