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