[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 25 13:32:53 PDT 2018
aaron.ballman added inline comments.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:37
+
+enum class LengthHandleKind { LHK_Increase, LHK_Decrease };
+
----------------
Please drop the `LHK_` prefix -- the enum class name is sufficient as a prefix.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:44
+// therefore we have to inject '+ 1UL' instead.
+static bool IsInjectUL = false;
+static std::map<StringRef, int> MacroDefinedMap;
----------------
This seems like it should be a parameter threaded through the function calls from check() rather than a static data member.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:55
+// The destination array is where the result of the function call will be stored
+static const Expr *getDestExpr(const MatchFinder::MatchResult &Result) {
+ return Result.Nodes.getNodeAs<Expr>(DestExprName);
----------------
These functions don't really add much benefit given that they're one-liners where the implementation is shorter than the declaration.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:75
+
+// Length is could be an IntegerLiteral or length of a StringLiteral
+static int getLength(const Expr *E, const MatchFinder::MatchResult &Result);
----------------
Length is could -> Length could
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:93
+
+// If the capacity of the destination array is not known it denoted as unknown
+static bool isKnownDest(const MatchFinder::MatchResult &Result) {
----------------
it denoted as unknown -> it is denoted as unknown.
(Be sure to add the full stop at the end of the sentence.)
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:101
+// Note: If the capacity and the given length is equal then the new function
+// is a simple 'cpy()' and because it returns true it prevents to increase
+// the given length.
----------------
prevents to increase -> prevents increasing
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:106
+
+// If we write/read from the same array it should be already null-terminated
+static bool isDestAndSrcEquals(const MatchFinder::MatchResult &Result);
----------------
Missing the full stop at the end of the sentence.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:110
+// We catch integers as a given length so we have to see if the length of the
+// source array is the same length and that is not null-terminated
+static bool isNoWrongLength(const MatchFinder::MatchResult &Result);
----------------
that is not -> that it is not
Missing the full stop at the end of the sentence.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:111
+// source array is the same length and that is not null-terminated
+static bool isNoWrongLength(const MatchFinder::MatchResult &Result);
+
----------------
isNoWrongLength should probably be named isNotWrongLength() or perhaps reverse the logic to isCorrectLength().
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:118
+
+static bool isGivenLengthEQToSrcLength(const MatchFinder::MatchResult &Result);
+
----------------
EQ -> Equal, but perhaps "lenEquaSrcLen()" is more succinct?
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:131
+
+// Increase or decrease by one an expression
+static void lengthExprHandle(LengthHandleKind LengthHandle,
----------------
Reorder the words and add a full stop: Increase or decrease an integral expression by one.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:137
+
+// Increase or decrease by one a passed argument by its position
+static void lengthArgHandle(LengthHandleKind LengthHandle, int ArgPos,
----------------
Similar: Increase or decrease the passed integral argument by one.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:143
+// If the destination array is the same length as the given length we have to
+// increase the capacity by one to create space for the the null terminator
+static bool destCapacityFix(const MatchFinder::MatchResult &Result,
----------------
Missing full stop.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:196
+ const Expr *SimpleNode = &Node;
+ SimpleNode = SimpleNode->IgnoreParenCasts()->IgnoreImpCasts();
+
----------------
You can call `IgnoreParenImpCasts()` instead.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:201
+
+ const auto DREHasInit = ignoringImpCasts(
+ declRefExpr(to(varDecl(hasInitializer(ignoringImpCasts(InnerMatcher))))));
----------------
You can remove the local `const` qualifications here (we don't typically use that for local unless the qualified type is a pointer or a reference).
Same elsewhere in the patch.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:268
+ //===--------------------------------------------------------------------===//
+ // The following six case match problematic length expressions
+ //===--------------------------------------------------------------------===//
----------------
case -> cases
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:331
+ //===--------------------------------------------------------------------===//
+ // The following five case match the 'destination' array length's
+ // expression which is used in memcpy() and memmove() matchers.
----------------
case -> cases
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:383
+ hasRHS(ignoringImpCasts(
+ anyOf(characterLiteral(equals(static_cast<unsigned>(0))),
+ integerLiteral(equals(0))))));
----------------
You can use `0U` here instead of an explicit cast.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:387
+ //===--------------------------------------------------------------------===//
+ // The following nineteen case match problematic function calls.
+ //===--------------------------------------------------------------------===//
----------------
case -> cases
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:459-460
+ if (It != MacroDefinedMap.end())
+ SafeFunctionsAvailable = It->second ? SafeFunctionsAvailableKind::Yes
+ : SafeFunctionsAvailableKind::No;
+ else
----------------
This is insufficient. Just because the user *wants* the safe functions doesn't mean they're actually available. You also need to check that `__STDC_LIB_EXT1__` is defined, as that tells you the implementation has Annex K functionality. If that's defined and the user also defines `__STDC_WANT_LIB_EXT1__` before including C standard library headers, then they can access them.
I am betting our include fixer doesn't properly handle ensuring the correct macros are defined before including the headers, but I think that's acceptable for now.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:514
+
+ // If it is cannot be rewritten to string handler function
+ if (Result.Nodes.getNodeAs<Type>(NotJustCharTyName)) {
----------------
it is cannot -> it cannot
Also, add a full stop at the end of the sentence.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:569-571
+ NewFuncName = (Name[0] != 'w') ? "str" : "wcs";
+ NewFuncName += IsCpy ? "cpy" : "ncpy";
+ NewFuncName += IsSafe ? "_s" : "";
----------------
This code is duplicated from above -- it'd be nice to refactor it into one place, perhaps.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:595-596
+ auto Diag = diag(FuncExpr->getArg(2)->IgnoreParenCasts()->getBeginLoc(),
+ "the length is too short for the last %0")
+ << ((Name[0] != 'w') ? "\'\\0\'" : "L\'\\0\'");
+
----------------
I think it's just as understandable, but more simple, to say "the length is too short to include the null terminator".
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:700
+ StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName();
+ MacroDefinedMap[MacroName] = IntValue.getZExtValue();
+ }
----------------
Is there a way we can do this without storing duplicate information? This is already tracked by the `Preprocessor` instance, I believe, so if we had a way to query that rather than store our own list, that would be better.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:715
+static bool isDestAndSrcEquals(const MatchFinder::MatchResult &Result) {
+ if (const auto DestVD = Result.Nodes.getNodeAs<Decl>(DestVarDeclName))
+ if (const auto *SrcVD = Result.Nodes.getNodeAs<Decl>(SrcVarDeclName))
----------------
`const auto *`
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:733
+
+ if (const auto DestTy = Result.Nodes.getNodeAs<ArrayType>(DestArrayTyName))
+ if (const auto *DestVAT = dyn_cast_or_null<VariableArrayType>(DestTy))
----------------
`const auto *`
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:843
+ // Assume that it cannot overflow if the expression of the destination
+ // capacity contains '+ 1'
+ if (DestCapacityExprStr.contains("+1") || DestCapacityExprStr.contains("+ 1"))
----------------
Missing full stop.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:871-872
+ dyn_cast_or_null<BinaryOperator>(LengthExpr->IgnoreParenCasts())) {
+ const auto *LHSExpr = OuterOpExpr->getLHS();
+ const auto *RHSExpr = OuterOpExpr->getRHS();
+ const auto *InnerOpExpr =
----------------
Do not use `auto` when the type is not spelled out in the initialization.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:879-883
+ const auto LHSRemoveRange =
+ SourceRange(LengthExpr->getBeginLoc(),
+ InnerOpExpr->getBeginLoc().getLocWithOffset(-1));
+ const auto RHSRemoveRange =
+ SourceRange(exprLocEnd(InnerOpExpr, Result), LengthExpr->getEndLoc());
----------------
These should not be declared as auto -- can skip the assignment operator. Same suggestion applies elsewhere.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:962
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ const auto *LengthExpr = FuncExpr->getArg(ArgPos)->IgnoreImpCasts();
+ lengthExprHandle(LengthHandle, LengthExpr, Result, Diag);
----------------
Do not use auto here either.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:983-984
+ const auto *FuncExpr = Result.Nodes.getNodeAs<CallExpr>(FuncExprName);
+ const auto ArgToRemove = FuncExpr->getArg(ArgPos);
+ const auto LHSArg = FuncExpr->getArg(ArgPos - 1);
+ const auto RemoveArgFix = FixItHint::CreateRemoval(
----------------
Don't use `auto` (elsewhere as well).
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038
+
+ SmallString<128> NewAddNullTermExprStr;
+ NewAddNullTermExprStr = "\n";
----------------
This should be done using a `Twine`.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.h:42-45
+ // - If "Default", "d", or "" (empty string) the checker relies on the
+ // '__STDC_WANT_LIB_EXT1__' macro being defined.
+ // - If "Yes", "y", or non-zero the '_s' suffixed functions are available.
+ // - If "No", "n", or 0 (zero) the '_s' suffixed functions are not available.
----------------
These comments are out of date -- you don't need "Yes", just Y or y (e.g.) as the first character. So "Yolo" will also work. :-P
================
Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:9
+this expression, because the null terminator needs an extra space. Without the
+null terminator it can result in an undefined behaviour when the string is read.
+
----------------
Drop "an".
https://reviews.llvm.org/D45050
More information about the cfe-commits
mailing list