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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
