[PATCH] D35068: [analyzer] Detect usages of unsafe I/O functions

[Kristóf Umann via Phabricator via cfe-commits]

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