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

Mon Jan 24 04:41:28 PST 2022

whisperity added a reviewer: whisperity.
whisperity added a comment.
Herald added a subscriber: rnkovacs.

Bump! Thanks @futogergely for picking up @ktomi996's work, and @steakhal for the help given during the implementation process!

In D91000#3231417 <https://reviews.llvm.org/D91000#3231417>, @martong wrote:

> In D91000#3230055 <https://reviews.llvm.org/D91000#3230055>, @futogergely wrote:
>
>> I think for now it is enough to issue a warning of using these functions, and not suggest a replacement. Should we add an option to the checker to also check for these functions?
>
> IMHO, it is okay to start with just simply issuing the warning. Later we might add that option (in a subsequent patch).

Yes, please. And I suggest indeed hiding it behind an option, e.g. //`ReportMoreUnsafeFunctions`// which I believe should default to true, but setting it to false would allow people who want their code to be "only" strict CERT-clean (that is, not care about these additional matches), can request doing so. Not offering a replacement right now is okay too, we can definitely work them in later.

In D91000#3197851 <https://reviews.llvm.org/D91000#3197851>, @aaron.ballman wrote:

> 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.

(Minor suggestion, but we could pull a trick with perhaps checking whether the macro is defined in the TU by the user and the library, and trying to match against the non-`std::` Annex K functions, and if they are available, consider the implementation the user is running under as "AnnexK-capable-even-in-C++-mode" and start issuing warnings? This might sound very technical, and definitely for later consideration!)

> 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.

Except for warning about `gets()` and suggesting `fgets()`?

----

Some nits: when it comes to inline comments, please mark the comments of the reviewers as "Done" when replying if you consider that comment was handled. Only you, the patch author, can do this. When there are too many open threads of comments, it gets messy to see what has been handled and what is still under discussion.

================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:41-42
+
+  // Matching the `gets` deprecated function without replacement.
+  auto DeprecatedFunctionNamesMatcher = hasAnyName("::gets");
+
----------------
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.

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

================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:94
+                                    FunctionNamesWithAnnexKReplacementId);
+  assert((Normal && !AnnexK) || (!Normal && AnnexK));
+
----------------

================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:110
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  ObsolescentFunctionsCheck::PP = PP;
+}
----------------
Maybe say `this->PP` or rename the parameter here `ThePP` and do `PP = ThePP`?

================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:19
+/// Checks for deprecated and obsolescent function listed in
+/// CERT C Coding Standard Recommendation MSC24 - C. For the listed functions,
+/// an alternative, safe replacement is suggested if available.
----------------

================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:21-24
+/// The checker heavily relies on the availability of annexK(Bounds - checking
+/// interfaces) from C11. For the user-facing documentation see:
+///
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc24-c.html
----------------
(Something went wrong with the formatting here.)

================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:42-43
+
+  // Used for caching the result of useSafeFunctionsFromAnnexK.
+  mutable llvm::Optional<bool> IsAnnexKAvailable;
+};
----------------

================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:95-98
+  Checks for deprecated and obsolescent functions listed in
+  CERT C Coding Standard Recommendation MSC24-C. For the listed functions,
+  an alternative, more secure replacement is suggested, if available. The checker heavily
+  relies on the functions from Annex K (Bounds-checking interfaces) of C11.
----------------
Format, reflow, align with the comment in the header, etc.

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

https://reviews.llvm.org/D91000