[PATCH] D91000: [clang-tidy] Add cert-msc24-c checker.

[Fütő Gergely via Phabricator via cfe-commits]

futogergely added a comment.

Maybe we could remove the check for setbuf() and rewind() functions, making this a pure Annex K checker. There is an overlapping with another recommendation (https://wiki.sei.cmu.edu/confluence/display/c/ERR07-C.+Prefer+functions+that+support+error+checking+over+equivalent+functions+that+don%27t), these functions are also listed there.



================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:41-42
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+
----------------
aaron.ballman wrote:
> This comment is not accurate. `gets_s()` is a secure replacement for `gets()`.
If gets is removed from C11, and gets_s is introduced in C11, then gets_s cannot be a replacement or? Maybe fgets?

Also I was wondering if we would like to disable this check for C99, maybe we should remove the check for gets all together.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:49-59
+      "::asctime", "::bsearch", "::ctime", "::fopen", "::fprintf", "::freopen",
+      "::fscanf", "::fwprintf", "::fwscanf", "::getenv", "::gmtime",
+      "::localtime", "::mbsrtowcs", "::mbstowcs", "::memcpy", "::memmove",
+      "::printf", "::qsort", "::snprintf", "::sprintf", "::sscanf", "::strcat",
+      "::strcpy", "::strerror", "::strncat", "::strncpy", "::strtok",
+      "::swprintf", "::swscanf", "::vfprintf", "::vfscanf", "::vfwprintf",
+      "::vfwscanf", "::vprintf", "::vscanf", "::vsnprintf", "::vsprintf",
----------------
aaron.ballman wrote:
> This list appears to be missing quite a few functions with secure replacements in Annex K. For example: `tmpfile_s`, `tmpnam_s`, `strerrorlen_s`, `strlen_s`... can you check the list against the actual Annex K, as it seems the CERT recommendation is still out of date.
Missing functions added: tmpfile/tmpfile_s, tmpnam/tmpnam_s, memset/memset_s, scanf, strlen, wcslen


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:84-86
+    diag(Range.getBegin(),
+         "function '%0' is deprecated as of C99, removed from C11.")
+        << Deprecated->getName() << Range;
----------------
aaron.ballman wrote:
> Fixed a few nits with the code, but `gets()` was never deprecated, so the message is not correct (it was present in C99 and removed in C11 with no deprecation period). I think it may be better to say "function %0 was removed in C11".
Done


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:103
+
+  diag(Range.getBegin(), "function '%0' %1; '%2' should be used instead.")
+      << FunctionName << getRationale(FunctionName)
----------------
aaron.ballman wrote:
> 
Done.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:18
+
+/// Checks for deprecated and obsolescent function listed in
+/// CERT C Coding Standard Recommendation MSC24 - C. For the listed functions,
----------------
aaron.ballman wrote:
> The terminology used in the CERT recommendation is pretty unfortunate and I don't think we should replicate it. Many of these are *not* deprecated or obsolescent functions and calling them that will confuse users. The crux of the CERT recommendation is that these functions have better replacements in more modern versions of C. So I would probably try to focus our diagnostics and documentation around modernization rather than deprecation.
> 
> FWIW, this is feedback that should also go onto the CERT recommendation itself. I noticed someone already observed the same thing: https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions?focusedCommentId=215482395#comment-215482395
Changed it to:
Checks for functions listed in CERT C Coding Standard Recommendation MSC24-C
that have safer, more secure replacements. The functions checked are considered unsafe because for
example they are missing bounds checking and/or non-reentrant. For the listed functions a
replacement function is suggested, if available. The checker heavily relies on the functions from
Annex K (Bounds-checking interfaces) of C11.

Also I changed the "is obsolescent" to "is not bounds-checking" in the getRationale function


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

https://reviews.llvm.org/D91000