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

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

futogergely marked 14 inline comments as done and an inline comment as not done.
futogergely added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:41-42
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+
----------------
whisperity wrote:
> whisperity wrote:
> > futogergely wrote:
> > > 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.
> > Yes, it's strange territory. If I make my code safer but //stay// pre-C11, I actually can't, because the new function isn't there yet. If I also //upgrade// then I'll **have to** make my code "safer", because the old function is now missing...
> > 
> > Given how brutally dangerous `gets` is (you very rarely see a documentation page just going **`Never use gets()!`**), I would suggest keeping at least the warning.
> > 
> > Although even the CERT rule mentions `gets_s()`. We could have some wiggle room here: do the warning for `gets()`, and suggest two alternatives: `fgets()`, or `gets_s()` + Annex K? `fgets(stdin, ...)` is also safe, if the buffer's size is given appropriately.
> CERT mentions C99 TC3, which //seems to be// available here: https://webstore.iec.ch/p-corrigenda/isoiec9899-cor3%7Bed2.0%7Den.pdf . I'm not sure how normative this source is (`iec.ch` seems legit to me that this isn't just a random WGML draft!), and on page 8, in point 46 it says: //"Add new paragraph after paragraph 1: The `gets` function is obsolescent, and is deprecated."//.
> 
> This seems like nitpicking, but maybe CppReference is outdated as it never indicated the "deprecation" period?
> 
> (FYI: http://www.iso.org/standard/50510.html does not offer any purchase of the older version of the standard, or this errata.)
I don't use 'deprecated' in the warning message, I issue 'is insecure, and it is removed from C11' for gets instead.

I added the following replacement suggestions for gets: gets_s if Annex K is available, fgets if Annex K is not available.


================
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",
----------------
futogergely wrote:
> 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
strlen/strnlen_s, wcslen/wcsnlen_s, memset/memset_s, scanf/scanf_s has been added.

I did not add tmpfile/tmpfile_s, tmpnam/tmpnam_s because there is a separate CERT rule for it: https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories.


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

https://reviews.llvm.org/D91000