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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
