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

Thu Jan 17 10:17:57 PST 2019

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

In D35068#811436 <https://reviews.llvm.org/D35068#811436>, @NoQ wrote:

> I wonder how noisy this check is - did you test it on large codebases? Because these functions are popular, and in many cases it'd be fine to use insecure functions, i wonder if it's worth it to have this check on by default. Like, if it's relatively quiet - it's fine, but if it'd constitute 90% of the analyzer's warnings on popular projects, that'd probably not be fine.

In D35068#1049530 <https://reviews.llvm.org/D35068#1049530>, @george.karpenkov wrote:

> @koldaniel Have you evaluated this checker? On which codebases? Were the warnings real security issues, or were they mostly spurious? The code seems fine, but I'm not sure whether it should be in `security` or in `alpha`.

Sorry, didn't read the discussion, there are some fair points in the quoted comments.

In D35068#1069880 <https://reviews.llvm.org/D35068#1069880>, @koldaniel wrote:

> I've evaluated this checker on LLVM+Clang, there were only a few (about 15) warnings,  because of the C11 flag check at the beginning of the checker body. However, if this check was removed, number of the warnings would be increased significantly. I wouldn't say the findings were real security issues, most of the warnings were about usages of deprecated functions, which has not been considered unsecure (but which may cause problems if the code is modified in an improper way in the future).

My problem is that LLVM+Clang isn't really a C (neither a C11) project, and I think judging this checker on it is a little misleading. Could you please test it on some C11 projects? I think tmux uses C11.

In D35068#1361195 <https://reviews.llvm.org/D35068#1361195>, @xazax.hun wrote:

> I think this is quiet coding guideline specific check which is useful for a set of  security critical projects. As this is an opt in kind of check, I think it does no harm to have it upstream.

I do generally agree with this statement, but I'd be more comfortable either landing it in alpha or seeing some other results.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D35068/new/

https://reviews.llvm.org/D35068