[clang-tidy] add new checker for string literal with NUL character.

[etienneb]

Summary

This patch adds the support for detecting suspicious string
literals and their incorrect usage.

The following example shows a incorrect character escaping leading
to an embedded NUL character.

std::string str = "\0x42";   // Should be "\x42".

The patch also add detection of truncated literal when a literal
is passed to a string constructor.

[etienneb]

Fix list.rst (bad merge)

[bkramer]

clang-tidy/misc/StringLiteralWithEmbeddedNulCheck.cpp

21 ↗

(On Diff #52653)

Isn't this identical to StringLiteral::getCodeUnit? Also returning 0 for unknown sizes is not a good idea imo.

[etienneb]

fix comments.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst.

[etienneb]

add missing reference in release-notes.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

161 ↗

(On Diff #52708)

I think will be good idea to sort check alphabetically. At least I did so :-)

[etienneb]

alpha ordering

[bcraig]

Is this checker able to connect a std::string with a pre-declared string literal (i.e. constant propagation)? For example...

const char *bad_chars = "\0\1\2\3";
std::string bad_str = bad_chars;

I've had real code make that mistake before.

[etienneb]

In D18783#392486, @bcraig wrote:

Is this checker able to connect a std::string with a pre-declared string literal (i.e. constant propagation)? For example...

const char *bad_chars = "\0\1\2\3";
std::string bad_str = bad_chars;

I've had real code make that mistake before.

Not yet, but I already started working in it.
There is more false-positive to remove first. It will follow this patch soon.

[alexfh]

A couple of nits for now. Will take a closer look later.

clang-tidy/misc/StringLiteralWithEmbeddedNulCheck.cpp

22 ↗

(On Diff #52710)

Should this be an assert instead?

38 ↗

(On Diff #52710)

clang-format -style=LLVM (or -style=file), please.

[etienneb]

alexfh comments.

[etienneb]

nits.

[etienneb]

I finally get rid of the GetCharAt function.

clang-tidy/misc/StringLiteralWithEmbeddedNulCheck.cpp

22 ↗

(On Diff #52710)

No, because we are not doing bounds check later:

if (GetCharAt(SL, i) == 0 &&
    GetCharAt(SL, i + 1) == 'x' &&
    isDigit(GetCharAt(SL, i + 2)) &&
    isDigit(GetCharAt(SL, i + 3))) {

We can move this as an assert, and in this case... we can modify the "if" to check the length.

[LegalizeAdulthood]

There are some APIs where a series of strings are passed as a single char * with NUL separators between the strings and the last string is indicated by two NUL characters in a row. For instance, see the MSDN documentation for the lpDependencies argument to [[ https://msdn.microsoft.com/en-us/library/windows/desktop/ms682450(v=vs.85).aspx | CreateService ]]. Such strings literals in the code look like:

LPCTSTR dependencies = _T("one\0two\0three\0four\0");

I don't see any annotation on this argument in <winsvc.h> where this function is declared, so there's no hint from the header about this argument. In this case, the biggest mistake people make is omitting the final NUL character, thinking that the \0 acts as a string separator and not a terminator:

LPCTSTR dependencies = _T("one\0two\0three\0four");

If you're lucky this results in a crash right away. If you're unlucky the compiler placed this constant in an area of memory with zero padding and it works by accident on your machine until it ships to a customer and then breaks on their machine mysteriously.

I was never a fan of these magic string array arguments and they mostly appear in the older portions of the Win32 API, but they are there nonetheless. I would hate for this check to signal a bunch of false positives on such strings.

[etienneb]

In D18783#393367, @LegalizeAdulthood wrote:

There are some APIs where a series of strings are passed as a single char * with NUL separators between the strings and the last string is indicated by two NUL characters in a row. For instance, see the MSDN documentation for the lpDependencies argument to [[ https://msdn.microsoft.com/en-us/library/windows/desktop/ms682450(v=vs.85).aspx | CreateService ]]. Such strings literals in the code look like:

LPCTSTR dependencies = _T("one\0two\0three\0four\0");

I don't see any annotation on this argument in <winsvc.h> where this function is declared, so there's no hint from the header about this argument. In this case, the biggest mistake people make is omitting the final NUL character, thinking that the \0 acts as a string separator and not a terminator:

LPCTSTR dependencies = _T("one\0two\0three\0four");

If you're lucky this results in a crash right away. If you're unlucky the compiler placed this constant in an area of memory with zero padding and it works by accident on your machine until it ships to a customer and then breaks on their machine mysteriously.

I was never a fan of these magic string array arguments and they mostly appear in the older portions of the Win32 API, but they are there nonetheless. I would hate for this check to signal a bunch of false positives on such strings.

I know about these kind functions, and they are not covered by the current state of this check.
The current check only apply if there is a conversion from char* -> std::string (implicit constructor).
For now, I want to keep the false-positive ratio low.

I made a prototype to detect instances passed to 'func(char*)' but there are too many false-positives.

[etienneb]

rebased.

[alexfh]

A couple of nits.

clang-tidy/misc/MisplacedWideningCastCheck.cpp

64

If clang-format did this, it used an incorrect style. In LLVM style it should put return on the next line. It should do the right thing, if you run it with -style=file and your clang-tools-extra working copy is checked out inside the cfe working copy (the important part here is that there's clang's or llvm's .clang-format file in a parent directory of the files you're running clang-format on).

95

s/RelativeIntSizes/relativeIntSize/ (singular, first character should be lower-case).

223

Two spaces before comments are common for Google style, so your clang-format most certainly uses a wrong style.

[alexfh]

Wait, it looks like you've updated the patch from an incorrect branch: I see only clang-tidy/misc/MisplacedWideningCastCheck.cpp file here.

[etienneb]

Revert last diff (invalid patchset).

[etienneb]

nit

[alexfh]

docs/clang-tidy/checks/misc-string-literal-with-embedded-nul.rst

6 ↗

(On Diff #52927)

Please use third-person form (Finds, validates, etc.).

27 ↗

(On Diff #52927)

s/an/a/

30 ↗

(On Diff #52927)

There doesn't seem to be anything bad with NUL-terminated. Did you mean with embedded NUL character?

[etienneb]

fix doc.

[alexfh]

LG with one nit.

clang-tidy/misc/StringLiteralWithEmbeddedNulCheck.cpp

70 ↗

(On Diff #52930)

nit: Should this be return instead?

[etienneb]

nit

clang-tidy/misc/MisplacedWideningCastCheck.cpp

 //===--- MisplacedWideningCastCheck.cpp - clang-tidy-----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MisplacedWideningCastCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "llvm/ADT/DenseMap.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 MisplacedWideningCastCheck::MisplacedWideningCastCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CheckImplicitCasts(Options.get("CheckImplicitCasts", true)) {}
 
 void MisplacedWideningCastCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckImplicitCasts", CheckImplicitCasts);
 }
 
 void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) {
-  auto Calc = expr(anyOf(binaryOperator(anyOf(
-                             hasOperatorName("+"), hasOperatorName("-"),
-                             hasOperatorName("*"), hasOperatorName("<<"))),
-                         unaryOperator(hasOperatorName("~"))),
-                   hasType(isInteger()))
-                  .bind("Calc");
+  const auto Calc =
+      expr(anyOf(binaryOperator(
+                     anyOf(hasOperatorName("+"), hasOperatorName("-"),
 
-  auto ExplicitCast =
+  const auto ExplicitCast =
       explicitCastExpr(hasDestinationType(isInteger()), has(Calc));
-  auto ImplicitCast =
+  const auto ImplicitCast =
       implicitCastExpr(hasImplicitDestinationType(isInteger()), has(Calc));
-  auto Cast = expr(anyOf(ExplicitCast, ImplicitCast)).bind("Cast");
+  const auto Cast = expr(anyOf(ExplicitCast, ImplicitCast)).bind("Cast");
 
   Finder->addMatcher(varDecl(hasInitializer(Cast)), this);
   Finder->addMatcher(returnStmt(hasReturnValue(Cast)), this);
   Finder->addMatcher(callExpr(hasAnyArgument(Cast)), this);
   Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this);
   Finder->addMatcher(
       binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!="),
                            hasOperatorName("<"), hasOperatorName("<="),
                            hasOperatorName(">"), hasOperatorName(">=")),
-                     anyOf(hasLHS(Cast), hasRHS(Cast))),
+                     hasEitherOperand(Cast)),
       this);
 }
 
-static unsigned getMaxCalculationWidth(ASTContext &Context, const Expr *E) {
+static unsigned getMaxCalculationWidth(const ASTContext &Context,
+                                       const Expr *E) {
   E = E->IgnoreParenImpCasts();
 
   if (const auto *Bop = dyn_cast<BinaryOperator>(E)) {
     unsigned LHSWidth = getMaxCalculationWidth(Context, Bop->getLHS());
     unsigned RHSWidth = getMaxCalculationWidth(Context, Bop->getRHS());
-    if (Bop->getOpcode() == BO_Mul)
-      return LHSWidth + RHSWidth;
-    if (Bop->getOpcode() == BO_Add)
-      return std::max(LHSWidth, RHSWidth) + 1;
+    if (Bop->getOpcode() == BO_Mul) return LHSWidth + RHSWidth;
+    if (Bop->getOpcode() == BO_Add) return std::max(LHSWidth, RHSWidth) + 1;
     if (Bop->getOpcode() == BO_Rem) {
       llvm::APSInt Val;
       if (Bop->getRHS()->EvaluateAsInt(Val, Context))
         return Val.getActiveBits();
     } else if (Bop->getOpcode() == BO_Shl) {
       llvm::APSInt Bits;
       if (Bop->getRHS()->EvaluateAsInt(Bits, Context)) {
         // We don't handle negative values and large values well. It is assumed
         // that compiler warnings are written for such values so the user will
         // fix that.
         return LHSWidth + Bits.getExtValue();
       }
 
       // Unknown bitcount, assume there is truncation.
       return 1024U;
     }
   } else if (const auto *Uop = dyn_cast<UnaryOperator>(E)) {
     // There is truncation when ~ is used.
-    if (Uop->getOpcode() == UO_Not)
-      return 1024U;
+    if (Uop->getOpcode() == UO_Not) return 1024U;
 
     QualType T = Uop->getType();
     return T->isIntegerType() ? Context.getIntWidth(T) : 1024U;
   } else if (const auto *I = dyn_cast<IntegerLiteral>(E)) {
     return I->getValue().getActiveBits();
   }
 
   return Context.getIntWidth(E->getType());
 }
 
-static llvm::SmallDenseMap<int, int> createRelativeIntSizesMap() {
-  llvm::SmallDenseMap<int, int> Result;
-  Result[BuiltinType::UChar] = 1;
-  Result[BuiltinType::SChar] = 1;
-  Result[BuiltinType::Char_U] = 1;
-  Result[BuiltinType::Char_S] = 1;
-  Result[BuiltinType::UShort] = 2;
-  Result[BuiltinType::Short] = 2;
-  Result[BuiltinType::UInt] = 3;
-  Result[BuiltinType::Int] = 3;
-  Result[BuiltinType::ULong] = 4;
-  Result[BuiltinType::Long] = 4;
-  Result[BuiltinType::ULongLong] = 5;
-  Result[BuiltinType::LongLong] = 5;
-  Result[BuiltinType::UInt128] = 6;
-  Result[BuiltinType::Int128] = 6;
-  return Result;
-}
-
-static llvm::SmallDenseMap<int, int> createRelativeCharSizesMap() {
-  llvm::SmallDenseMap<int, int> Result;
-  Result[BuiltinType::UChar] = 1;
-  Result[BuiltinType::SChar] = 1;
-  Result[BuiltinType::Char_U] = 1;
-  Result[BuiltinType::Char_S] = 1;
-  Result[BuiltinType::Char16] = 2;
-  Result[BuiltinType::Char32] = 3;
-  return Result;
-}
-
-static llvm::SmallDenseMap<int, int> createRelativeCharSizesWMap() {
-  llvm::SmallDenseMap<int, int> Result;
-  Result[BuiltinType::UChar] = 1;
-  Result[BuiltinType::SChar] = 1;
-  Result[BuiltinType::Char_U] = 1;
-  Result[BuiltinType::Char_S] = 1;
-  Result[BuiltinType::WChar_U] = 2;
-  Result[BuiltinType::WChar_S] = 2;
-  return Result;
+static int RelativeIntSizes(BuiltinType::Kind kind) {
+  switch (kind) {
+    case BuiltinType::UChar:
+      return 1;
+    case BuiltinType::SChar:
+      return 1;
+    case BuiltinType::Char_U:
+      return 1;
+    case BuiltinType::Char_S:
+      return 1;
+    case BuiltinType::UShort:
+      return 2;
+    case BuiltinType::Short:
+      return 2;
+    case BuiltinType::UInt:
+      return 3;
+    case BuiltinType::Int:
+      return 3;
+    case BuiltinType::ULong:
+      return 4;
+    case BuiltinType::Long:
+      return 4;
+    case BuiltinType::ULongLong:
+      return 5;
+    case BuiltinType::LongLong:
+      return 5;
+    case BuiltinType::UInt128:
+      return 6;
+    case BuiltinType::Int128:
+      return 6;
+    default:
+      return 0;
+  }
+}
+
+static int RelativeCharSizes(BuiltinType::Kind kind) {
+  switch (kind) {
+    case BuiltinType::UChar:
+      return 1;
+    case BuiltinType::SChar:
+      return 1;
+    case BuiltinType::Char_U:
+      return 1;
+    case BuiltinType::Char_S:
+      return 1;
+    case BuiltinType::Char16:
+      return 2;
+    case BuiltinType::Char32:
+      return 3;
+    default:
+      return 0;
+  }
+}
+
+static int RelativeCharSizesW(BuiltinType::Kind kind) {
+  switch (kind) {
+    case BuiltinType::UChar:
+      return 1;
+    case BuiltinType::SChar:
+      return 1;
+    case BuiltinType::Char_U:
+      return 1;
+    case BuiltinType::Char_S:
+      return 1;
+    case BuiltinType::WChar_U:
+      return 2;
+    case BuiltinType::WChar_S:
+      return 2;
+    default:
+      return 0;
+  }
 }
 
 static bool isFirstWider(BuiltinType::Kind First, BuiltinType::Kind Second) {
-  static const llvm::SmallDenseMap<int, int> RelativeIntSizes(
-      createRelativeIntSizesMap());
-  static const llvm::SmallDenseMap<int, int> RelativeCharSizes(
-      createRelativeCharSizesMap());
-  static const llvm::SmallDenseMap<int, int> RelativeCharSizesW(
-      createRelativeCharSizesWMap());
-
   int FirstSize, SecondSize;
-  if ((FirstSize = RelativeIntSizes.lookup(First)) &&
-      (SecondSize = RelativeIntSizes.lookup(Second)))
+  if ((FirstSize = RelativeIntSizes(First)) != 0 &&
+      (SecondSize = RelativeIntSizes(Second)) != 0)
     return FirstSize > SecondSize;
-  if ((FirstSize = RelativeCharSizes.lookup(First)) &&
-      (SecondSize = RelativeCharSizes.lookup(Second)))
+  if ((FirstSize = RelativeCharSizes(First)) != 0 &&
+      (SecondSize = RelativeCharSizes(Second)) != 0)
     return FirstSize > SecondSize;
-  if ((FirstSize = RelativeCharSizesW.lookup(First)) &&
-      (SecondSize = RelativeCharSizesW.lookup(Second)))
+  if ((FirstSize = RelativeCharSizesW(First)) != 0 &&
+      (SecondSize = RelativeCharSizesW(Second)) != 0)
     return FirstSize > SecondSize;
+
   return false;
 }
 
 void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Cast = Result.Nodes.getNodeAs<CastExpr>("Cast");
-  if (!CheckImplicitCasts && isa<ImplicitCastExpr>(Cast))
-    return;
-  if (Cast->getLocStart().isMacroID())
-    return;
+  if (!CheckImplicitCasts && isa<ImplicitCastExpr>(Cast)) return;
+  if (Cast->getLocStart().isMacroID()) return;
 
   const auto *Calc = Result.Nodes.getNodeAs<Expr>("Calc");
-  if (Calc->getLocStart().isMacroID())
-    return;
+  if (Calc->getLocStart().isMacroID()) return;
 
-  ASTContext &Context = *Result.Context;
+  const ASTContext &Context = *Result.Context;
 
   QualType CastType = Cast->getType();
   QualType CalcType = Calc->getType();
 
   // Explicit truncation using cast.
-  if (Context.getIntWidth(CastType) < Context.getIntWidth(CalcType))
-    return;
+  if (Context.getIntWidth(CastType) < Context.getIntWidth(CalcType)) return;
 
   // If CalcType and CastType have same size then there is no real danger, but
   // there can be a portability problem.
-
   if (Context.getIntWidth(CastType) == Context.getIntWidth(CalcType)) {
     const auto *CastBuiltinType =
         dyn_cast<BuiltinType>(CastType->getUnqualifiedDesugaredType());
     const auto *CalcBuiltinType =
         dyn_cast<BuiltinType>(CalcType->getUnqualifiedDesugaredType());
     if (CastBuiltinType && CalcBuiltinType &&
-        !isFirstWider(CastBuiltinType->getKind(), CalcBuiltinType->getKind()))
+        !isFirstWider(CastBuiltinType->getKind(), CalcBuiltinType->getKind())) {
       return;
     }
+  }
 
   // Don't write a warning if we can easily see that the result is not
   // truncated.
   if (Context.getIntWidth(CalcType) >= getMaxCalculationWidth(Context, Calc))
     return;
 
-  diag(Cast->getLocStart(), "either cast from %0 to %1 is ineffective, or "
-                            "there is loss of precision before the conversion")
+  diag(Cast->getLocStart(),
+       "either cast from %0 to %1 is ineffective, or "
       << CalcType << CastType;
 }
 
-} // namespace misc
-} // namespace tidy
-} // namespace clang
+}  // namespace misc
+}  // namespace tidy
+}  // namespace clang