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