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,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('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("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 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("this-dest")) { + + diag(ThisDest->getLocStart(), "memset used on this pointer potentially " + "corrupts virtual method table"); + } +} + +} // 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,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: test/clang-tidy/misc-suspicious-memset-usage.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-suspicious-memset-usage.cpp @@ -0,0 +1,107 @@ +// RUN: %check_clang_tidy %s misc-suspicious-memset-usage %t + +//void *memset(void *, int, __SIZE_TYPE__); +#include +template +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(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)); } + }; + } +};