[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Quoting the revision summary:

> This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
`asctime`, `atof`, `atoi`, `atol`, `atoll`, `ctime`, `fopen`, `freopen`, `rewind`, `setbuf`

Hm, I get that `cert-err34-c` will already diagnose the uses of `atof`, `atoi`, `atol`, `atoll`, but then why do we check `vfscanf`, `vscanf` then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking `asctime`, `ctime`, `fopen`, `freopen`, `rewind`, `setbuf` for the sake of completeness.
What do you think?

There is a mismatch between your code and docs too, regarding the function you check for.
In the code you match for `vsprintf`, `vsscanf`, `vswprintf`, `vswcanf`, `wcrtomb`, `wcscat`, but none of these are mentioned in the docs.

> The checker warns only if `STDC_LIB_EXT1` macro is defined and the value of `STDC_WANT_LIB_EXT1` macro is `1` in this case it suggests the corresponding functions from Annex K instead the vulnerable function.

I would suggest mentioning these macros and their **purpose** in the docs.
Eg. that the `STDC_WANT_LIB_EXT1` should be defined to `1` but the other is left to the implementation.

That being said, I would request more tests, demonstrating that this macro detection works accordingly.

This checker might be a bit noisy. Have you tried it on open-source projects?
If it is, we should probably note that in the docs as well.

In the tests, It is a good practice to demonstrate that the offered recommendation does not trigger yet another warning.
Don't forget to put a `no-warning` to highlight that.



================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:30
+      "::vfprintf", "::vfscanf", "vfwprintf", "vfwscanf", "vprintf", "::vscanf",
+      "::vsnprintf", "::vsprintf", "::vsscanf", "::vswprintf", "::vswcanf",
+      "::wcrtomb", "::wcscat", "::wcscpy", "::wcsncat", "::wcsncpy",
----------------
Hm, it must be a typo.
`vswcanf` -> `vswscanf`

By the same token, I miss these functions from this enumeration.
`vfwprintf`, `vfwscanf`, `vprintf`, `vwprintf`, `vwscanf`
However, they are present in the cert table you mentioned.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:33
+      "::wcsrtombs", "::wcstok", "::wcstombs", "::wctomb", "::wmemcpy",
+      "::wmemmove", "::wprintf", "::wscanf", "::strlen"));
+  Finder->addMatcher(
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:69
+    AreSafeFunctionsWanted = IntValue.getZExtValue();
+    if (AreSafeFunctionsWanted.hasValue()) {
+      AnnexKIsWanted = AreSafeFunctionsWanted.getValue();
----------------
Eugene.Zelenko wrote:
> Please elide braces.
Please also check if the value is `1`, as your summary and the standard requires.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:77-79
+  std::pair<bool, bool> AnnexKUsage = UsingAnnexK();
+  bool AnnexKIsAvailable = std::get<0>(AnnexKUsage);
+  bool AnnexKIsWanted = std::get<1>(AnnexKUsage);
----------------



================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:87-89
+    diag(Function->getExprLoc(), "Unsafe function " + FunctionName + " used. " +
+                                     "Safe " + FunctionName +
+                                     "_s can be used instead.");
----------------
We should probably use `llvm::Twine` instead of multiple `std::string` constructions.
I might be wrong though.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:89
+                                     "Safe " + FunctionName +
+                                     "_s can be used instead.");
+  }
----------------
`can` -> `should`
The same at the other diagnostic message.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:91
+  }
+  if (const auto *FunctionPointer = Result.Nodes.getNodeAs<VarDecl>("fp")) {
+    if (const FunctionDecl *FD = Result.Nodes.getNodeAs<FunctionDecl>("fnp")) {
----------------
This and the previous `if` statement is exclusive, am I right?
If we have a match, that is **either** a callexpr **or** a vardecl.

I would prefer a `return` at the end of the previous `if` and an assert here that the `FunctionPointer` must be non-null.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:27
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  const std::pair<bool, bool> UsingAnnexK();
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
----------------
It should be private.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:10
+    strcat, strcpy, strerror, strncat, strncpy, strtok, swprintf, swscanf,
+    vfprintf, vfscanf, vfwprintf, vfwscanf, vprintf, vscanf vsnprintf, vspr,
+    wcscpy, wcsncat, wcsncpy wcsrtombs, wcstok, wcstombs, wctomb, wmemcpy,
----------------
Another typo.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:19-23
+        #define __STDC_WANT_LIB_EXT1__ 1
+        int i = 2;
+        int j = 3;
+        memmove(&i, &j, sizeof(int)); // diagnosed if __STDC_LIB_EXT1__ is defined in a
+                                    // header, memmove_s usage is suggested instead.
----------------
I'm not convinced by this example.

AFAIK, the intended usage is to define the `__STDC_WANT_LIB_EXT1__` to `1` before the `string.h` header included.
It's important to provide a good example, as this checker will be silent if not used correctly.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:23
+        memmove(&i, &j, sizeof(int)); // diagnosed if __STDC_LIB_EXT1__ is defined in a
+                                    // header, memmove_s usage is suggested instead.
----------------
We should probably align this comment as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000