Szelethus accepted this revision.
Szelethus added a comment.
Overall I think this looks great, thanks! I left some inlines that would be
nice to fix before commiting, but all of them are minor nits.
Would it be possible for you to commit the clang-formatting and the actual
logic separately?
================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:393
+ def DeprecatedOrUnsafeBufferHandling :
Checker<"DeprecatedOrUnsafeBufferHandling">,
+ HelpText<"Warn on uses of unsecure or deprecated buffer manipulating
functions">,
+ DescFile<"CheckSecuritySyntaxOnly.cpp">;
----------------
Lately we started paying attention to the 80 column limit in `Checkers.td`.
Could you please break this line?
================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:583
+// Use of 'sprintf', 'vsprintf', 'scanf', 'wscanf',
+//'fscanf',
+// 'fwscanf', 'vscanf', 'vwscanf', 'vfscanf', 'vfwscanf', 'sscanf',
----------------
This is out of place.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:591
+//===----------------------------------------------------------------------===//
+void WalkAST::checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE,
+ const FunctionDecl *FD) {
----------------
Could you please add an extra newline?
================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:598
//===----------------------------------------------------------------------===//
-
void WalkAST::checkCall_rand(const CallExpr *CE, const FunctionDecl *FD) {
----------------
I think this newline should stay.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:603-610
+ int ArgIndex =
+ llvm::StringSwitch<int>(Name)
+ .Cases("scanf", "wscanf", "vscanf", "vwscanf", 0)
+ .Cases("sprintf", "vsprintf", "fscanf", "fwscanf", "vfscanf",
+ "vfwscanf", "sscanf", "swscanf", "vsscanf", "vswscanf", 1)
+ .Cases("swprintf", "snprintf", "vswprintf", "vsnprintf", "memcpy",
+ "memmove", "memset", "strncpy", "strncat", DEPR_ONLY)
----------------
I wonder whether using `CallDescription` would be (way) more efficient here.
Comparing `IndentifierInfo`s would also be better I guess.
I'm a little unsure about performance implications here, it might not be worth
the chore to refactor this.
================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:627-630
+ SmallString<128> buf1;
+ SmallString<512> buf2;
+ llvm::raw_svector_ostream out1(buf1);
+ llvm::raw_svector_ostream out2(buf2);
----------------
Could you please start these variable names with a capital letter?
================
Comment at: test/Analysis/security-syntax-checks.m:253
+ FILE *file;
+ sprintf(buf, "a"); // expected-warning{{Call to function 'sprintf' is
insecure as it does not provide security checks introduced in the C11 standard.
Replace with analogous functions that support length arguments or provides
boundary checks such as 'sprintf_s' in case of C11}}
+ scanf("%d", &a); // expected-warning{{Call to function 'scanf' is insecure
as it does not provide security checks introduced in the C11 standard. Replace
with analogous functions that support length arguments or provides boundary
checks such as 'scanf_s' in case of C11}}
----------------
When using `{{}}`, you actually supply a regex as an argument, and the output
of the analyzer is matched against it. My point is, could you instead just write
```
// expected-warning{{Call to function 'sprintf' is insecure}}
```
to improve readability?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D35068/new/
https://reviews.llvm.org/D35068
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
