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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun 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">,
----------------
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? 


================
Comment at: lib/Driver/ToolChains/Clang.cpp:2256
+      CmdArgs.push_back("-analyzer-checker=security.insecureAPI.DeprecatedOrUnsafeBufferHandling");
+      CmdArgs.push_back("-analyzer-checker=security.insecureAPI.DeprecatedBufferHandling");
     }
----------------
Do we want to turn on both of these by default? Warning on every use of `memset` will be very noisy for example. 


================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:597-598
+
+  if(!BR.getContext().getLangOpts().C11)
+    return;
+
----------------
koldaniel wrote:
> NoQ wrote:
> > Note that you cannot easily figure out if the code is intended to get compiled only under C11 and above - maybe it's accidentally compiled under C11 for this user, but is otherwise intended to keep working under older standards.
> It is a possible scenario, how should I check if the checks should warn (safe functions are available) if not by using this method?
This is the same approach some check takes in clang tidy. I think it is still better to not warn for non C11 users than warn for everybody. If a project is not interested in this check but they are interested in having C11 builds, they can turn this check off. (Or it can be off by default in the first place).


================
Comment at: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:147
+    .Case("sprintf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vsprintf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("scanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
----------------
This is the only print-related function in this group. Is this intended? 


https://reviews.llvm.org/D35068