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 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( "misc-assert-side-effect"); CheckFactories.registerCheck("misc-misplaced-const"); + CheckFactories.registerCheck( + "misc-suspicious-memset-usage"); CheckFactories.registerCheck( "misc-unconventional-assign-operator"); CheckFactories.registerCheck( 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,97 @@ +//===--- 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('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("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("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("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 Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -72,6 +72,11 @@ Finds perfect forwarding constructors that can unintentionally hide copy or move constructors. +- New `misc-suspicious-memset-usage + `_ check + + Finds ``memset()`` calls with potential mistakes in their arguments. + - New `modernize-replace-random-shuffle `_ check 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: docs/clang-tidy/checks/misc-suspicious-memset-usage.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-suspicious-memset-usage.rst @@ -0,0 +1,54 @@ +.. 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 + } + }; Index: test/clang-tidy/misc-suspicious-memset-usage.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-suspicious-memset-usage.cpp @@ -0,0 +1,126 @@ +// RUN: %check_clang_tidy %s misc-suspicious-memset-usage %t + +void *memset(void *, int, __SIZE_TYPE__); + +template +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(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)); + } + }; +};