[clang-tidy] Add bugprone-suspicious-memset-usage check.

[rnkovacs]

Summary

Created new module bugprone and placed the check in that.

Finds memset() calls with potential mistakes in their arguments.

Replaces and extends the existing google-runtime-memset-zero-length check. Cases covered:

• Fill value is a character '0'. Integer 0 might have been intended.
• Fill value is out of char range and gets truncated.
• Byte count is zero. Potentially swapped with the fill value argument.

Hits on LLVM codebase:

susp-memset-llvm.txt816 BDownload

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/misc-suspicious-memset-usage.rst

7

Please enclose memset on `` and add ().

26

Please enclose memset on `` and add ().

[malcolm.parsons]

The destination is a this pointer within a class that has a virtual function. It might reset the virtual pointer.

Can you change this to match any pointer to a class with a virtual function?

I'd also like a warning for a pointer to a class with a constructor or destructor.

[rnkovacs]

• Fixed function call format in docs.
• Added check to release notes.

[rnkovacs]

In D32700#743178, @malcolm.parsons wrote:

Can you change this to match any pointer to a class with a virtual function?

Well, I have just found a clang flag [[ https://clang.llvm.org/docs/DiagnosticsReference.html#wdynamic-class-memaccess | -Wdynamic-class-memaccess ]] that finds all of these cases and is enabled by default. I guess I need to drop the this case entirely.

I'd also like a warning for a pointer to a class with a constructor or destructor.

This is a nice idea, I'll do it.

[rnkovacs]

• Removed case related to virtual pointers as there is a diagnostic flag for that.
• Added case warning for calls on classes with a constructor or destructor. Changed tests and docs accordingly.

[xazax.hun]

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

22

I think this might not be the best approach.

For example, if the constructor is compiler generated, but there is a member of the class with non-trivial constructor, we still want to warn.

E.g.:

struct X { X() { /* something nontrivial */ } };

struct Y { X x; };

Maybe we should check instead whether the class is a POD? Other alternative might be something like
CXXRecordDecl::hasNonTrivialDefaultConstructor.

[rnkovacs]

The current proposition could be that we only keep the first two cases, possibly merging in the google check for a third case (with its old name evoking original functionality). Separately, another check could be written that warns when the below mentioned memory management functions are used on not TriviallyCopyable objects.

I wonder if any of the commenters/reviewers/subscribers have any thoughts about this :)

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

22

So, we had a discussion yesterday that I'll try to sum up here. The root of the problem is not exactly about constructors or the object being a POD, but rather about calling memset() on an object that is not TriviallyCopyable. But as you suggested, this holds for memcpy() and memmove() as well, and might be better placed in another check.

[alexfh]

The existing google-runtime-memset-zero-length check is related. It finds cases when the byte count parameter is zero and offers to swap that with the fill value argument. Perhaps the two could be merged while maintaining backward compatibility through an alias.

I think, the checks should be merged, and there's no need to maintain the old behavior under the google-runtime-memset-zero-length alias. I'm not even sure we want to make this alias, as long as the new check isn't ridiculously noisy.

At this point I'd suggest that we go back to the idea of adding a separate module for checks that target patterns that are likely to be bugs rather than performance, readability or style issues. The name could be "bugprone" or something like this, and we could move many misc- checks there eventually.

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

22

This logic could be either here or in a separate check (covering 'memcpy', 'memmove' and friends), but it might even be reasonable to create a compiler diagnostic for this eventually (maybe after a test drive of the logic in a clang-tidy check).

[rnkovacs]

• Merged in the google-runtime-memset-zero-length check.
• Added a separate check for memory manipulation functions: [[ https://reviews.llvm.org/D35051 | misc-undefined-memory-manipulation ]].

[rnkovacs]

• Created new module bugprone and moved the check into that.
• Included summary of hits on LLVM in the description.

[whisperity]

Considering the results published in the opening description:

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:55:29: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]

std::memset(NumberBuffer, '0', sizeof(NumberBuffer));
                          ^~~~
                          0

/home/reka/codechecker_dev_env/llvm/lib/Support/NativeFormatting.cpp:148:26: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage]

::memset(NumberBuffer, '0', llvm::array_lengthof(NumberBuffer));
                       ^~~~
                       0

In these case, the buffer is of char[] type. This memset call initialises the array with the character literal ASCII 0, which is, in this context, the expected behaviour. Maybe you should add an exemption from the checker for these cases, i.e. where we do know that the buffer is a char[].

Tests should be extended with a case for this addition.

[alexfh]

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

127–130 ↗

(On Diff #106172)

It looks like this can be replaced with clang::tooling::fixit::getText() or even createReplacement.

[rnkovacs]

• Added char[] exception along with a test case. There are no more false positives on LLVM.
• Simplified fix-its by using clang::tooling::fixit functions.

[whisperity]

docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst

10 ↗

(On Diff #106268)

Shouldn't this '0' be enclosed within backticks?

[rnkovacs]

docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst

10 ↗

(On Diff #106268)

Fixed that and the too short title underline.

[alexfh]

Looks good with a single comment.

Thank you for working on this!

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

74–75 ↗

(On Diff #106381)

The formatting is rather misleading here. Let's put the comment inside the if. Same below.

[whisperity]

clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp

57–58 ↗

(On Diff #106381)

@alexfh Your review on putting the comments within their applicable branch bodies applies here too?

89–90 ↗

(On Diff #106381)

Same as L57-58.

[rnkovacs]

Moved comments inside if bodies.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   MisplacedConstCheck.cpp
+  SuspiciousMemsetUsageCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringCompareCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
+#include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"

 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
+    CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
+        "misc-suspicious-memset-usage");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>("misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<FoldInitTypeCheck>("misc-fold-init-type");

clang-tidy/misc/SuspiciousMemsetUsageCheck.h

+//===--- SuspiciousMemsetUsageCheck.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_SUSPICIOUS_MEMSET_USAGE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_MEMSET_USAGE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds memset calls with potential mistakes in their arguments.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-suspicious-memset-usage.html
+class SuspiciousMemsetUsageCheck : public ClangTidyCheck {
+public:
+  SuspiciousMemsetUsageCheck(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_SUSPICIOUS_MEMSET_USAGE_H

clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp

+//===--- SuspiciousMemsetUsageCheck.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 "SuspiciousMemsetUsageCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void SuspiciousMemsetUsageCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ClassWVirtualFunc = cxxRecordDecl(
+      anyOf(hasMethod(isVirtual()),
+            isDerivedFrom(cxxRecordDecl(hasMethod(isVirtual())))));
+
+  const auto MemsetThisCall =
+      callExpr(callee(functionDecl(hasName("::memset"))),
+               hasArgument(0, cxxThisExpr().bind("this-dest")),
+               unless(isInTemplateInstantiation()));
+
+  Finder->addMatcher(
+      callExpr(
+          callee(functionDecl(hasName("::memset"))),
+          hasArgument(1, characterLiteral(equals(static_cast<unsigned>('0')))
+                             .bind("char-zero-fill")),
+          unless(hasArgument(0, hasType(pointsTo(isAnyCharacter()))))),
+      this);
+
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("::memset"))),
+               hasArgument(1, integerLiteral().bind("num-fill"))),
+      this);
+
+  Finder->addMatcher(
+      cxxMethodDecl(
+          hasParent(ClassWVirtualFunc),
+          anyOf(hasDescendant(MemsetThisCall),
+                hasDescendant(lambdaExpr(hasDescendant(MemsetThisCall)))),
+          unless(hasDescendant(cxxRecordDecl(hasDescendant(MemsetThisCall))))),
+      this);
+}
+
+void SuspiciousMemsetUsageCheck::check(const MatchFinder::MatchResult &Result) {
+  // Case 1: Fill value of memset is a character '0'.
+  // Possibly an integer zero was intended.
+  if (const auto *CharZeroFill =
+          Result.Nodes.getNodeAs<CharacterLiteral>("char-zero-fill")) {
+
+    SourceRange CharRange = CharZeroFill->getSourceRange();
+    auto Diag =
+        diag(CharZeroFill->getLocStart(), "memset fill value is char '0', "
+                                          "potentially mistaken for int 0");
+
+    // Only suggest a fix if no macros are involved.
+    if (CharRange.getBegin().isMacroID())
+      return;
+    Diag << FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(CharRange), "0");
+  }
+
+  // Case 2: Fill value of memset is larger in size than an
+  // unsigned char, which means it gets truncated during conversion.
+  else if (const auto *NumFill =
+               Result.Nodes.getNodeAs<IntegerLiteral>("num-fill")) {
+
+    llvm::APSInt NumValue;
+    const auto UCharMax = (1 << Result.Context->getCharWidth()) - 1;
+    if (!NumFill->EvaluateAsInt(NumValue, *Result.Context) ||
+        (NumValue >= 0 && NumValue <= UCharMax))
+      return;
+
+    diag(NumFill->getLocStart(), "memset fill value is out of unsigned "
+                                 "character range, gets truncated");
+  }
+
+  // Case 3: Destination pointer of memset is a this pointer.
+  // If the class containing the memset call has a virtual method,
+  // memset is likely to corrupt the virtual method table.
+  // Inner structs form an exception.
+  else if (const auto *ThisDest =
+               Result.Nodes.getNodeAs<CXXThisExpr>("this-dest")) {
+
+    diag(ThisDest->getLocStart(), "memset used on this pointer potentially "
+                                  "corrupts virtual method table");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- Improved `cppcoreguidelines-no-malloc
 
   Allow custom memory management functions to be considered as well.
   
 - New `misc-forwarding-reference-overload
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-forwarding-reference-overload.html>`_ check
 
   Finds perfect forwarding constructors that can unintentionally hide copy or move constructors.
 
+- New `misc-suspicious-memset-usage
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-suspicious-memset-usage.html>`_ check
+
+  Finds ``memset()`` calls with potential mistakes in their arguments.
+
 - New `modernize-replace-random-shuffle
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-random-shuffle.html>`_ check
 
   Finds and fixes usage of ``std::random_shuffle`` as the function has been removed from C++17.
 
 - New `modernize-return-braced-init-list
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html>`_ check
 

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-sizeof-container
    misc-sizeof-expression
    misc-static-assert
    misc-string-compare
    misc-string-constructor
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-enum-usage
+   misc-suspicious-memset-usage
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-suspicious-string-compare
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-unconventional-assign-operator
    misc-undelegated-constructor
    misc-uniqueptr-reset-release

docs/clang-tidy/checks/misc-suspicious-memset-usage.rst

+.. title:: clang-tidy - misc-suspicious-memset-usage
+
+misc-suspicious-memset-usage
+============================
+
+This check finds ``memset()`` calls with potential mistakes in their arguments.
+Considering the function as ``void* memset(void* destination, int fill_value,
+size_t byte_count)``, the following cases are covered:
+
+**Case 1: Fill value is a character '0'**
+
+Filling up a memory area with ASCII code 48 characters is not customary,
+possibly integer zeroes were intended instead.
+The check offers a replacement of ``'0'`` with ``0``. Memsetting character
+pointers with ``'0'`` is allowed.
+
+**Case 2: Fill value is truncated**
+
+Memset converts ``fill_value`` to ``unsigned char`` before using it. If
+``fill_value`` is out of unsigned character range, it gets truncated
+and memory will not contain the desired pattern.
+
+**Case 3: Destination is a this pointer**
+
+If the class containing the ``memset()`` call has a virtual function, using
+memset on the ``this`` pointer might corrupt the virtual method table.
+Inner structs form an exception.
+
+Examples:
+
+.. code-block:: c++
+
+  void SuspiciousFillValue() {
+
+    int i[5] = {1, 2, 3, 4, 5};
+    int *ip = i;
+    int l = 5;
+    char c = '1';
+    char *cp = &z;
+
+    // Case 1
+    memset(ip, '0', l); // suspicious
+    memset(cp, '0', 1); // OK
+
+    // Case 2
+    memset(ip, 0xabcd, l); // fill value gets truncated
+    memset(ip, 0x00, l);   // OK
+
+  }
+
+  // Case 3
+  class WithVirtualFunction() {
+    virtual void f() {}
+    WithVirtualFunction() {
+      memset(this, 0, sizeof(*this)); // might reset virtual pointer
+    }
+  }

test/clang-tidy/misc-suspicious-memset-usage.cpp

+// RUN: %check_clang_tidy %s misc-suspicious-memset-usage %t
+
+//void *memset(void *, int, __SIZE_TYPE__);
+#include <cstring>
+template <typename T>
+void mtempl(int* ptr) {
+  memset(ptr, '0', sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is char '0', potentially mistaken for int 0 [misc-suspicious-memset-usage]
+  // CHECK-FIXES: memset(0, 0, sizeof(T));
+  memset(ptr, 256, sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is out of unsigned character range, gets truncated [misc-suspicious-memset-usage]
+}
+
+void BadFillValue() {
+
+  int i[5] = {1, 2, 3, 4, 5};
+  int *p = i;
+  int l = 5;
+  char z = '1';
+  char *c = &z;
+
+  memset(p, '0', l);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is char '0', potentially mistaken for int 0 [misc-suspicious-memset-usage]
+  // CHECK-FIXES: memset(p, 0, l);
+#define M memset(p, '0', l);
+  M
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is char '0', potentially mistaken for int 0 [misc-suspicious-memset-usage]
+
+  // Valid expressions:
+  memset(p, '2', l);
+  memset(p, 0, l);
+  memset(c, '0', 1);
+
+  memset(p, 0xabcd, l);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is out of unsigned character range, gets truncated [misc-suspicious-memset-usage]
+
+  // Valid expressions:
+  memset(p, 0x00, l);
+  mtempl<int>(p);
+}
+
+class A {
+  virtual void a1() {}
+  void a2() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on this pointer potentially corrupts virtual method table [misc-suspicious-memset-usage]
+  }
+public:
+  A() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on this pointer potentially corrupts virtual method table [misc-suspicious-memset-usage]
+  }
+};
+
+class B : public A {
+  void b1() {}
+  void b2() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on this pointer potentially corrupts virtual method table [misc-suspicious-memset-usage]
+  }
+};
+
+int main() {
+  A var {};
+  memset(&var, 0, 5);
+}
+
+// Valid expressions:
+class C {
+  void c1() {}
+  void c2() {
+    memset(this, 0, sizeof(*this));
+  }
+  C() {
+    memset(this, 0, sizeof(*this));
+  }
+};
+
+class D {
+  virtual void d1() {}
+  void d2() {
+    [this] { memset(this, 0, sizeof(*this)); };
+    // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memset used on this pointer potentially corrupts virtual method table [misc-suspicious-memset-usage]
+  }
+  D() {
+    [this] { memset(this, 0, sizeof(*this)); };
+    // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memset used on this pointer potentially corrupts virtual method table [misc-suspicious-memset-usage]
+  }
+};
+
+// Valid expressions:
+class E {
+  virtual void e1() {}
+  E() {
+    struct S1 {
+      void s1() { memset(this, 0, sizeof(*this)); }
+    };
+  }
+  struct S2 {
+    void s2() { memset(this, 0, sizeof(*this)); }
+  };
+  void e2() {
+    struct S3 {
+      void s3() { memset(this, 0, sizeof(*this)); }
+    };
+  }
+};