[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

21

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

21

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

21

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

   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   MisplacedConstCheck.cpp
+  SuspiciousMemsetUsageCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
+#include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"

     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>(

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 HasCtorOrDtor =
+      eachOf(hasMethod(cxxConstructorDecl(unless(anyOf(
+                 isCopyConstructor(), isMoveConstructor(), isImplicit())))),
+             hasMethod(cxxDestructorDecl(unless(isImplicit()))));
+
+  Finder->addMatcher(
+      callExpr(
+          callee(functionDecl(hasName("::memset"))),
+          hasArgument(1, characterLiteral(equals(static_cast<unsigned>('0')))
+                             .bind("char-zero-fill")),
+          unless(eachOf(hasArgument(0, hasType(pointsTo(isAnyCharacter()))),
+                        isInTemplateInstantiation()))),
+      this);
+
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("::memset"))),
+                              hasArgument(1, integerLiteral().bind("num-fill")),
+                              unless(isInTemplateInstantiation())),
+                     this);
+
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("::memset"))),
+               hasArgument(0, hasType(pointsTo(cxxRecordDecl(anyOf(
+                                  HasCtorOrDtor, isDerivedFrom(cxxRecordDecl(
+                                                     HasCtorOrDtor))))))),
+               unless(isInTemplateInstantiation()))
+          .bind("call-ctor-dtor"),
+      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 is a class with a user-defined constructor
+  // or destructor. As memset() is only guaranteed to work on POD type
+  // objects, this should be avoided.
+  else if (const auto *Call =
+               Result.Nodes.getNodeAs<CallExpr>("call-ctor-dtor")) {
+
+    const Expr *Dest = Call->getArg(0);
+    diag(Dest->getLocStart(), "memset used on class with user-defined "
+                              "constructor or destructor");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 
   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
 

docs/clang-tidy/checks/list.rst

    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

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 class with a constructor/destructor**
+
+Calling ``memset()`` on a non-POD type object risks undefined behavior.
+The check warns if ``destination`` points to a class with a non-trivial,
+user-defined constructor or destructor.
+
+Examples:
+
+.. code-block:: c++
+
+  void SuspiciousFillValue() {
+    int i[5] = {1, 2, 3, 4, 5};
+    int *ip = i;
+    char c = '1';
+    char *cp = &z;
+
+    // Case 1
+    memset(ip, '0', 1); // suspicious
+    memset(cp, '0', 1); // OK
+
+    // Case 2
+    memset(ip, 0xabcd, 1); // fill value gets truncated
+    memset(ip, 0x00, 1);   // OK
+  }
+
+  // Case 3
+  struct NonTrivial {
+    NonTrivial();                     // user-defined constructor
+    void f() {
+      memset(this, 0, sizeof(*this)); // no guarantees
+    }
+  };

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

+// RUN: %check_clang_tidy %s misc-suspicious-memset-usage %t
+
+void *memset(void *, int, __SIZE_TYPE__);
+
+template <typename T>
+void mtempl(int *ptr) {
+  memset(ptr, '0', sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: memset fill value is char '0', potentially mistaken for int 0 [misc-suspicious-memset-usage]
+  // CHECK-FIXES: memset(ptr, 0, sizeof(T));
+  memset(ptr, 256, sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: memset fill value is out of unsigned character range, gets truncated [misc-suspicious-memset-usage]
+}
+
+void FillValue() {
+  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);
+  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]
+
+#define M_CHAR_ZERO memset(p, '0', l);
+  M_CHAR_ZERO
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is char '0', potentially mistaken for int 0 [misc-suspicious-memset-usage]
+#define M_OUTSIDE_RANGE memset(p, 0xabcd, l);
+  M_OUTSIDE_RANGE
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is out of unsigned character range, gets truncated [misc-suspicious-memset-usage]
+
+  memset(p, '2', l);
+  memset(p, 0, l);
+  memset(c, '0', 1);
+  memset(p, 0x00, l);
+  mtempl<int>(p);
+}
+
+struct Ctor {
+  Ctor();
+  void fCtor() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+};
+
+struct Dtor {
+  ~Dtor();
+  void fDtor() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+};
+
+struct Both {
+  Both();
+  ~Both();
+  void fBoth() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+};
+
+struct Neither {
+  void fNeither() {
+    memset(this, 0, sizeof(*this));
+  }
+};
+
+void Destination() {
+  Ctor a;
+  memset(&a, 0, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  Dtor b;
+  memset(&b, 0, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  Both c;
+  memset(&c, 0, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  Neither d;
+  memset(&d, 0, 1);
+}
+
+struct Inside {
+  Inside() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+  ~Inside() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+};
+
+struct Base {
+  Base();
+  ~Base();
+};
+
+struct Derived : public Base {
+  void fDerived() {
+    memset(this, 0, sizeof(*this));
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+};
+
+struct Lambda {
+  Lambda();
+  ~Lambda();
+  void fLambda() {
+    [this] { memset(this, 0, sizeof(*this)); };
+    // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memset used on class with user-defined constructor or destructor [misc-suspicious-memset-usage]
+  }
+};
+
+struct Outer {
+  Outer();
+  ~Outer();
+  struct Inner {
+    void fInner() {
+      memset(this, 0, sizeof(*this));
+    }
+  };
+};