[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

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

23

Should this be an assert instead?

39

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

23

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 ↗

(On Diff #52856)

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 ↗

(On Diff #52856)

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

223 ↗

(On Diff #52856)

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

7

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

28

s/an/a/

31

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

71

nit: Should this be return instead?

[etienneb]

nit

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
+  StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
+#include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "misc-string-integer-assignment");
+    CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
+        "misc-string-literal-with-embedded-nul");
     CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
         "misc-suspicious-missing-comma");
     CheckFactories.registerCheck<SuspiciousSemicolonCheck>(
         "misc-suspicious-semicolon");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");

clang-tidy/misc/StringLiteralWithEmbeddedNulCheck.h

+//===--- StringLiteralWithEmbeddedNulCheck.h - clang-tidy--------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_LITERAL_WITH_EMBEDDED_NUL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_LITERAL_WITH_EMBEDDED_NUL_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Find suspicious string literals with embedded NUL characters.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-literal-with-embedded-nul.html
+class StringLiteralWithEmbeddedNulCheck : public ClangTidyCheck {
+public:
+  StringLiteralWithEmbeddedNulCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_LITERAL_WITH_EMBEDDED_NUL_H

clang-tidy/misc/StringLiteralWithEmbeddedNulCheck.cpp

+//===--- StringLiteralWithEmbeddedNulCheck.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 "StringLiteralWithEmbeddedNulCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+// Retrieve the character at offset |offset| of the string literal |SL|.
+static unsigned int GetCharAt(const StringLiteral *SL, size_t offset) {
+  if (offset >= SL->getLength()) return 0;
+  switch (SL->getCharByteWidth()) {
+    case 1:
+      return SL->getBytes()[offset];
+    case 2:
+      return reinterpret_cast<const uint16_t *>(SL->getBytes().data())[offset];
+    case 4:
+      return reinterpret_cast<const uint32_t *>(SL->getBytes().data())[offset];
+    default:
+      return 0;
+  }
+}
+
+AST_MATCHER(StringLiteral, containsNul) {
+  for (size_t i = 0; i < Node.getLength(); ++i)
+    if (GetCharAt(&Node, i) == 0) return true;
+  return false;
+}
+
+void StringLiteralWithEmbeddedNulCheck::registerMatchers(MatchFinder *Finder) {
+  // Match a string that contains embedded NUL character. Extra-checks are
+  // applied in |check| to find incorectly escaped characters.
+  Finder->addMatcher(stringLiteral(containsNul()).bind("strlit"), this);
+
+  // The remaining checks only apply to C++.
+  if (!getLangOpts().CPlusPlus) return;
+
+  const auto StrLitWithNul =
+      ignoringParenImpCasts(stringLiteral(containsNul()).bind("truncated"));
+
+  // Match string constructor.
+  const auto StringConstructorExpr = expr(anyOf(
+      cxxConstructExpr(argumentCountIs(1),
+                       hasDeclaration(cxxMethodDecl(hasName("basic_string")))),
+      // If present, the second argument is the alloc object which must not
+      // be present explicitly.
+      cxxConstructExpr(argumentCountIs(2),
+                       hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+                       hasArgument(1, cxxDefaultArgExpr()))));
+
+  // Detect passing a suspicious string literal to a string constructor.
+  // example: std::string str = "abc\0def";
+  Finder->addMatcher(
+      cxxConstructExpr(StringConstructorExpr, hasArgument(0, StrLitWithNul)),
+      this);
+
+  // Detect passing a suspisious string literal through an overloaded operator.
+  Finder->addMatcher(cxxOperatorCallExpr(hasAnyArgument(StrLitWithNul)), this);
+}
+
+void StringLiteralWithEmbeddedNulCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *SL = Result.Nodes.getNodeAs<StringLiteral>("strlit")) {
+    for (size_t i = 0; i < SL->getLength(); ++i) {
+      // Find a sequence of character like "\0x12".
+      if (GetCharAt(SL, i) == 0 && GetCharAt(SL, i + 1) == 'x' &&
+          isDigit(GetCharAt(SL, i + 2)) && isDigit(GetCharAt(SL, i + 3))) {
+        diag(SL->getLocStart(), "suspicious embedded NUL character");
+        break;
+      }
+    }
+  }
+
+  if (const auto *SL = Result.Nodes.getNodeAs<StringLiteral>("truncated")) {
+    diag(SL->getLocStart(),
+         "truncated string literal with embedded NUL character");
+  }
+}
+
+}  // namespace misc
+}  // namespace tidy
+}  // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-string-integer-assignment
+   misc-string-literal-with-embedded-nul
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters

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

+.. title:: clang-tidy - misc-string-literal-with-embedded-nul
+
+misc-string-literal-with-embedded-nul
+=====================================
+
+Find occurences of string literal with embedded NUL character and validate
+their usage.
+
+
+Invalid escaping
+^^^^^^^^^^^^^^^^
+
+Special characters can be escaped within a string literal by using their
+hexadecimal encoding like ``\x42``. A common mistake is to escape them
+like this ``\0x42`` where the ``\0`` stands for the NUL character.
+
+.. code:: c++
+
+  const char* Example[] = "Invalid character: \0x12 should be \x12";
+  const char* Bytes[] = "\x03\0x02\0x01\0x00\0xFF\0xFF\0xFF";
+
+
+Truncated literal
+^^^^^^^^^^^^^^^^^
+
+String-like classes can manipulate strings with embedded NUL as they are
+keeping track of the bytes and the length. This is not the case for an
+``char*`` (NUL-terminated) string.
+
+A common mistake is to pass a string-literal (NUL-terminated) to a string
+constructor. The bytes after the first NUL character are truncated.
+
+.. code:: c++
+
+  std::string str("abc\0def");  // "def" is truncated
+  str += "\0";                  // This statement is doing nothing
+  if (str == "\0abc") return;   // This expression is always true
+
+

test/clang-tidy/misc-string-literal-with-embedded-nul.cpp

+// RUN: %check_clang_tidy %s misc-string-literal-with-embedded-nul %t
+
+namespace std {
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T, typename A>
+struct basic_string {
+  typedef basic_string<C, T, A> _Type;
+  basic_string();
+  basic_string(const C *p, const A &a = A());
+
+  _Type& operator+=(const C* s);
+  _Type& operator=(const C* s);
+};
+
+typedef basic_string<char, std::char_traits<char>, std::allocator<char>> string;
+typedef basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t>> wstring;
+}
+
+bool operator==(const std::string&, const char*);
+bool operator==(const char*, const std::string&);
+
+
+const char Valid[] = "This is valid \x12.";
+const char Strange[] = "This is strange \0x12 and must be fixed";
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: suspicious embedded NUL character [misc-string-literal-with-embedded-nul]
+
+const char textA[] = "\0x01\0x02\0x03\0x04";
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious embedded NUL character
+const wchar_t textW[] = L"\0x01\0x02\0x03\0x04";
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious embedded NUL character
+
+const char A[] = "\0";
+const char B[] = "\0x";
+const char C[] = "\0x1";
+const char D[] = "\0x11";
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: suspicious embedded NUL character
+
+const wchar_t E[] = L"\0";
+const wchar_t F[] = L"\0x";
+const wchar_t G[] = L"\0x1";
+const wchar_t H[] = L"\0x11";
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: suspicious embedded NUL character
+
+const char I[] = "\000\000\000\000";
+const char J[] = "\0\0\0\0\0\0";
+const char K[] = "";
+
+const char L[] = "\0x12" "\0x12" "\0x12" "\0x12";
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: suspicious embedded NUL character
+
+void TestA() {
+  std::string str1 = "abc\0def";
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: truncated string literal
+  std::string str2 = "\0";
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: truncated string literal
+  std::string str3("\0");
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: truncated string literal
+  std::string str4{"\x00\x01\x02\x03"};
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: truncated string literal
+
+  std::string str;
+  str += "abc\0def";
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: truncated string literal
+  str = "abc\0def";
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: truncated string literal
+
+  if (str == "abc\0def") return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: truncated string literal
+  if ("abc\0def" == str) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: truncated string literal
+}
+
+void TestW() {
+  std::wstring str1 = L"abc\0def";
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: truncated string literal
+  std::wstring str2 = L"\0";
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: truncated string literal
+  std::wstring str3(L"\0");
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: truncated string literal
+  std::wstring str4{L"\x00\x01\x02\x03"};
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: truncated string literal
+}
+