rnkovacs created this revision.
Herald added a subscriber: mgorny.
This check finds memset calls with potential mistakes in their arguments.
Cases covered:
- Fill value is a character '0'. Integer 0 might have been intended.
- Fill value is out of character range and gets truncated.
- The destination is a this pointer within a class that has a virtual function.
It might reset the virtual pointer.
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. When turned on using the alias name, the check
would only examine the count parameter as in the google check.
Any suggestions are appreciated.
https://reviews.llvm.org/D32700
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp
clang-tidy/misc/SuspiciousMemsetUsageCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-suspicious-memset-usage.rst
test/clang-tidy/misc-suspicious-memset-usage.cpp
Index: test/clang-tidy/misc-suspicious-memset-usage.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/misc-suspicious-memset-usage.cpp
@@ -0,0 +1,101 @@
+// RUN: %check_clang_tidy %s misc-suspicious-memset-usage %t
+
+void *memset(void *, int, __SIZE_TYPE__);
+
+template <typename T>
+void mtempl() {
+ memset(0, '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(0, 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>();
+}
+
+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]
+ }
+ 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]
+ }
+};
+
+// 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)); }
+ };
+ }
+};
Index: docs/clang-tidy/checks/misc-suspicious-memset-usage.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/misc-suspicious-memset-usage.rst
@@ -0,0 +1,57 @@
+.. 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
+ }
+ }
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
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
Index: clang-tidy/misc/SuspiciousMemsetUsageCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/misc/SuspiciousMemsetUsageCheck.h
@@ -0,0 +1,35 @@
+//===--- 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
Index: clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp
@@ -0,0 +1,99 @@
+//===--- 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
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -40,6 +40,7 @@
#include "StringIntegerAssignmentCheck.h"
#include "StringLiteralWithEmbeddedNulCheck.h"
#include "SuspiciousEnumUsageCheck.h"
+#include "SuspiciousMemsetUsageCheck.h"
#include "SuspiciousMissingCommaCheck.h"
#include "SuspiciousSemicolonCheck.h"
#include "SuspiciousStringCompareCheck.h"
@@ -66,6 +67,8 @@
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>(
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -4,6 +4,7 @@
ArgumentCommentCheck.cpp
AssertSideEffectCheck.cpp
MisplacedConstCheck.cpp
+ SuspiciousMemsetUsageCheck.cpp
UnconventionalAssignOperatorCheck.cpp
BoolPointerImplicitConversionCheck.cpp
DanglingHandleCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
