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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for this new check!

In terms of whether we should expose the check in C++: I think we shouldn't. Annex K *could* be supported by a C++ library implementation (http://eel.is/c++draft/library#headers-10), but none of the identifiers are added to namespace `std` and there's not a lot of Annex K support in C so I imagine there's even less support in C++. We can always revisit the decision later and expand support for C++ once we know what that support should look like.

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

We should also circle back with the CERT authors for updates on missing entries in their tables once we've figured out what is missing, and on the terminology concerns of referring to these as "deprecated or obsolescent" functions.



================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:33
+      "::wcsrtombs", "::wcstok", "::wcstombs", "::wctomb", "::wmemcpy",
+      "::wmemmove", "::wprintf", "::wscanf", "::strlen"));
+  Finder->addMatcher(
----------------
steakhal wrote:
> Why is `strlen` an //obsolescent// function? I don't feel it justified, even if there was a [[ https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions?focusedCommentId=88019519#comment-88019519 | comment ]] about it on the cert page.
I think its exclusion from the CERT page was an oversight. `strlen_s()` is a secure replacement for `strlen()` and it would be weird to leave it off the list.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:41-42
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+
----------------
This comment is not accurate. `gets_s()` is a secure replacement for `gets()`.


================
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",
----------------
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.


================
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;
----------------
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".


================
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)
----------------



================
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,
----------------
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


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

https://reviews.llvm.org/D91000