[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:21
+
+static std::string exprToStr(const Expr *Expr,
+                             const MatchFinder::MatchResult &Result) {
----------------
Do not name the parameter with the same name as a type. Same applies elsewhere in this file.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:21
+
+static std::string exprToStr(const Expr *Expr,
+                             const MatchFinder::MatchResult &Result) {
----------------
aaron.ballman wrote:
> Do not name the parameter with the same name as a type. Same applies elsewhere in this file.
Why returning a `std::string` rather than a `StringRef`? It seems like the callers don't always need the extra copy operation.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:36
+                                const MatchFinder::MatchResult &Result) {
+  auto NewExpr = const_cast<clang::Expr *>(Expr);
+  ParenExpr *PE = new (Result.Context)
----------------
Can you sink the `const_cast` into the only place it's required, and remove the `clang::` from it?


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:40
+                NewExpr->getLocEnd().getLocWithOffset(1), NewExpr);
+  return Expr == PE->getSubExpr();
+}
----------------
This seems really inefficient -- it's creating a new `ParentExpr` just to throw it away each time. Also, I'm not certain what it's accomplishing. The constructor for `ParenExpr` accepts an `Expr *` (in this case, `Expr`) which it returns from `ParenExpr::getSubExpr()`, so this looks like it's a noop that just tests `Expr == Expr`. What have I missed?


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:70
+  const auto LengthCall =
+      callExpr(callee(functionDecl(hasAnyName("strlen", "wcslen"))),
+               expr().bind("length-call"));
----------------
Please match on `::strlen` and `::wcslen` so that you don't accidentally catch random other function declarations.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:132-133
+
+  // clang-format off
+  const auto Alloca =     Matcher("alloca",     1, 0, StrLenKind::WithoutInc);
+  const auto Calloc =     Matcher("calloc",     2, 0, StrLenKind::WithoutInc);
----------------
Please use the fully-qualified names here as well. Also, I'm not keen on needing to shut off clang-format just to get the alignment to work below -- we don't typically try to keep things like this aligned unless there's a reason to need it (like making the code maintainable), which I don't think applies here.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:193
+      diag(Result.Nodes.getNodeAs<CallExpr>("expr")->getLocStart(),
+           "'%0' function's allocated memory cannot hold the null-terminator")
+      << Name;
----------------
`null terminator` (remove the hyphen).

Also, it might be nice to reformulate to drop the possessive apostrophe on "function's". Perhaps something like: `"memory allocated by '%0' is insufficient to hold the null terminator"`

Same elsewhere in this file.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:210
+    auto Diag = diag(Result.Nodes.getNodeAs<CallExpr>("expr")->getLocStart(),
+                     "'%0' function's result is not null-terminated")
+                << Name;
----------------
Similar here: `"the result from calling '%0' is not null-terminated"` (or something along those lines). Same elsewhere.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:226
+    DiagnosticBuilder &Diag) {
+  if (getLangOpts().CPlusPlus11) {
+    StringRef NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s";
----------------
What about C?


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:306-307
+  auto Diag = diag(Result.Nodes.getNodeAs<CallExpr>("expr")->getLocStart(),
+                   "'strerror_s' function's result is not null-terminated and "
+                   "missing the last character of the error message");
+  lengthArgInsertIncByOne(1, Result, Diag);
----------------
Similar rewording here to remove the possessive.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:321
+    auto Diag = diag(FuncExpr->getLocStart(),
+                     "'%0' function's comparison's length is too long")
+                << Name;
----------------
Given that this is a concern about the argument to the comparison function, I think this diagnostic should point to the argument rather than the function, and then it can drop the name of the function entirely to alleviate the awkward wording. Something like `"comparison length is too long and might lead to a buffer overflow"`.


================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.h:59
+  void
+  lengthArgInsertIncByOne(const int ArgPos,
+                          const ast_matchers::MatchFinder::MatchResult &Result,
----------------
No need for the argument to be a `const int` (same elsewhere) as the argument will be copied regardless.


https://reviews.llvm.org/D45050