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

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:382
+    DescFile<"CheckSecuritySyntaxOnly.cpp">;
+  def DeprecatedBufferHandling : Checker<"DeprecatedBufferHandling">,
+    HelpText<"Warn on uses of deprecated buffer manipulating functions">,
----------------
xazax.hun wrote:
> I do not like the naming of these two checks, It feels like one of them warns for a subset of the other, however, it is not the case.
> What about removing the "deprecated" part from the first check? 
Both checker warns if a buffer handling function is deprecated (DeprecatedOrUnsafeBufferHandling calls DeprecatedBufferHandling), but the DeprecatedOrUnsafeBufferHandling checker also warns if a function is not only deprecated but unsafe (i.e. writes a buffer without size restrictions) too.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:2256
+      CmdArgs.push_back("-analyzer-checker=security.insecureAPI.DeprecatedOrUnsafeBufferHandling");
+      CmdArgs.push_back("-analyzer-checker=security.insecureAPI.DeprecatedBufferHandling");
     }
----------------
xazax.hun wrote:
> Do we want to turn on both of these by default? Warning on every use of `memset` will be very noisy for example. 
It's true that these checkers could be very noisy if they are enabled by default, I'm going to fix this.


================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:147
+    .Case("sprintf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vsprintf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("scanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
----------------
xazax.hun wrote:
> This is the only print-related function in this group. Is this intended? 
sprintf and vsprintf are buffer-writing functions without size restrictions, this is the reason why only these two are checked by DeprecatedOrUnsafeBufferHandling of the printf family. There are buffer-writing members of this family which are not unsafe (because of size restrictions) but have a secure version introduced in the C11 standard, and DeprecatedBufferHandling warns for them: swprintf, snprintf, vswprintf, vsnprintf. Other printf-related functions are either printing to a file or to the standard output.


https://reviews.llvm.org/D35068