Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -42,6 +42,7 @@ #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" #include "SuspiciousEnumUsageCheck.h" +#include "SuspiciousMemoryComparisonCheck.h" #include "SuspiciousMemsetUsageCheck.h" #include "SuspiciousMissingCommaCheck.h" #include "SuspiciousSemicolonCheck.h" @@ -131,6 +132,8 @@ "bugprone-string-literal-with-embedded-nul"); CheckFactories.registerCheck( "bugprone-suspicious-enum-usage"); + CheckFactories.registerCheck( + "bugprone-suspicious-memory-comparison"); CheckFactories.registerCheck( "bugprone-suspicious-memset-usage"); CheckFactories.registerCheck( Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -34,6 +34,7 @@ StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp SuspiciousEnumUsageCheck.cpp + SuspiciousMemoryComparisonCheck.cpp SuspiciousMemsetUsageCheck.cpp SuspiciousMissingCommaCheck.cpp SuspiciousSemicolonCheck.cpp Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h @@ -0,0 +1,35 @@ +//===--- SuspiciousMemoryComparisonCheck.h - clang-tidy ---------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSMEMORYCOMPARISONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSMEMORYCOMPARISONCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds potentially incorrect calls to ``memcmp()`` that compare padding or +/// non-standard-layout classes. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-memory-comparison.html +class SuspiciousMemoryComparisonCheck : public ClangTidyCheck { +public: + SuspiciousMemoryComparisonCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSMEMORYCOMPARISONCHECK_H Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp @@ -0,0 +1,162 @@ +//===--- SuspiciousMemoryComparisonCheck.cpp - clang-tidy -----------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "SuspiciousMemoryComparisonCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +static constexpr llvm::StringLiteral CallExprName = "call"; + +static bool hasPadding(const clang::ASTContext &Ctx, const RecordDecl *RD, + uint64_t ComparedBits); + +static uint64_t getFieldSize(const clang::FieldDecl &FD, + clang::QualType FieldType, + const clang::ASTContext &Ctx) { + if (FD.isBitField()) + return FD.getBitWidthValue(Ctx); + return Ctx.getTypeSize(FieldType); +} + +static bool hasPaddingInBase(const clang::ASTContext &Ctx, const RecordDecl *RD, + uint64_t ComparedBits, uint64_t &TotalSize) { + const auto IsNotEmptyBase = [](const CXXBaseSpecifier &Base) { + return !Base.getType()->getAsCXXRecordDecl()->isEmpty(); + }; + + if (const auto *CXXRD = dyn_cast(RD)) { + const auto NonEmptyBaseIt = llvm::find_if(CXXRD->bases(), IsNotEmptyBase); + if (NonEmptyBaseIt != CXXRD->bases().end()) { + assert(llvm::count_if(CXXRD->bases(), IsNotEmptyBase) == 1 && + "RD is expected to be a standard layout type"); + + const CXXRecordDecl *BaseRD = + NonEmptyBaseIt->getType()->getAsCXXRecordDecl(); + const uint64_t SizeOfBase = Ctx.getTypeSize(BaseRD->getTypeForDecl()); + TotalSize += SizeOfBase; + + // check if comparing padding in base + if (hasPadding(Ctx, BaseRD, ComparedBits)) + return true; + } + } + + return false; +} + +static bool hasPaddingBetweenFields(const clang::ASTContext &Ctx, + const RecordDecl *RD, uint64_t ComparedBits, + uint64_t &TotalSize) { + for (const auto &Field : RD->fields()) { + const uint64_t FieldOffset = Ctx.getFieldOffset(Field); + assert(FieldOffset >= TotalSize); + + if (FieldOffset > TotalSize && TotalSize < ComparedBits) + // Padding before this field + return true; + + if (FieldOffset >= ComparedBits) + return false; + + const uint64_t SizeOfField = getFieldSize(*Field, Field->getType(), Ctx); + TotalSize += SizeOfField; + + if (Field->getType()->isRecordType()) { + // Check if comparing padding in nested record + if (hasPadding(Ctx, Field->getType()->getAsRecordDecl()->getDefinition(), + ComparedBits - FieldOffset)) + return true; + } + } + + return false; +} + +static bool hasPadding(const clang::ASTContext &Ctx, const RecordDecl *RD, + uint64_t ComparedBits) { + assert(RD && RD->isCompleteDefinition()); + if (RD->isUnion()) + return false; + + uint64_t TotalSize = 0; + + if (hasPaddingInBase(Ctx, RD, ComparedBits, TotalSize) || + hasPaddingBetweenFields(Ctx, RD, ComparedBits, TotalSize)) + return true; + + // check for trailing padding + return TotalSize < Ctx.getTypeSize(RD->getTypeForDecl()) && + ComparedBits > TotalSize; +} + +static llvm::Optional +tryEvaluateSizeExpr(const Expr *SizeExpr, const clang::ASTContext &Ctx) { + clang::Expr::EvalResult Result; + if (SizeExpr->EvaluateAsRValue(Result, Ctx)) + return Ctx.toBits( + CharUnits::fromQuantity(Result.Val.getInt().getExtValue())); + else + return None; +} + +void SuspiciousMemoryComparisonCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr(callee(namedDecl( + anyOf(hasName("::memcmp"), hasName("::std::memcmp"))))) + .bind(CallExprName), + this); +} + +void SuspiciousMemoryComparisonCheck::check( + const MatchFinder::MatchResult &Result) { + const clang::ASTContext &Ctx = *Result.Context; + const auto *CE = Result.Nodes.getNodeAs(CallExprName); + assert(CE != nullptr); + + const Expr *SizeExpr = CE->getArg(2); + assert(SizeExpr != nullptr); + const llvm::Optional ComparedBits = + tryEvaluateSizeExpr(SizeExpr, Ctx); + + for (unsigned int i = 0; i < 2; ++i) { + const Expr *ArgExpr = CE->getArg(i); + const QualType ArgType = ArgExpr->IgnoreImplicit()->getType(); + const Type *const PointeeType = ArgType->getPointeeOrArrayElementType(); + assert(PointeeType != nullptr); + + if (PointeeType->isRecordType()) { + const RecordDecl *RD = PointeeType->getAsRecordDecl()->getDefinition(); + if (RD != nullptr) { + if (const auto *CXXDecl = dyn_cast(RD)) { + if (!CXXDecl->isStandardLayout()) { + diag(CE->getBeginLoc(), + "comparing object representation of non-standard-layout " + "type %0; consider using a comparison operator instead") + << ArgType->getPointeeType(); + break; + } + } + + if (ComparedBits && hasPadding(Ctx, RD, *ComparedBits)) { + diag(CE->getBeginLoc(), "comparing padding bytes"); + break; + } + } + } + } +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "../bugprone/UnhandledSelfAssignmentCheck.h" #include "../bugprone/BadSignalToKillThreadCheck.h" +#include "../bugprone/SuspiciousMemoryComparisonCheck.h" #include "../google/UnnamedNamespaceInHeaderCheck.h" #include "../misc/NewDeleteOverloadsCheck.h" #include "../misc/NonCopyableObjects.h" @@ -80,6 +81,9 @@ "cert-dcl16-c"); // ENV CheckFactories.registerCheck("cert-env33-c"); + // EXP + CheckFactories.registerCheck( + "cert-exp42-c"); // FLP CheckFactories.registerCheck("cert-flp30-c"); // FIO Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -94,6 +94,17 @@ Without the null terminator it can result in undefined behaviour when the string is read. +- New :doc:`bugprone-suspicious-memory-comparison + ` check. + + Finds potentially incorrect calls to ``memcmp()`` that compare padding or + non-standard-layout classes. + +- New alias :doc:`cert-exp42-c + ` to + :doc:`bugprone-suspicious-memory-comparison + ` was added. + - New :doc:`cert-mem57-cpp ` check. Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst @@ -0,0 +1,14 @@ +.. title:: clang-tidy - bugprone-suspicious-memory-comparison + +bugprone-suspicious-memory-comparison +===================================== + +Finds potentially incorrect calls to ``memcmp()`` that compare padding or +non-standard-layout classes. + +This check corresponds to the CERT C Coding Standard rule +`EXP42-C. Do not compare padding data +`_. +and part of the CERT C++ Coding Standard rule +`OOP57-CPP. Prefer special member functions and overloaded operators to C Standard Library functions +`_. Index: clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst @@ -0,0 +1,9 @@ +.. title:: clang-tidy - cert-exp42-c +.. meta:: + :http-equiv=refresh: 5;URL=bugprone-suspicious-memory-comparison.html + +cert-exp42-c +============ + +The cert-exp42-c check is an alias, please see +`bugprone-suspicious-memory-comparison `_ for more information. Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -76,6 +76,7 @@ `bugprone-string-integer-assignment `_, "Yes", "low" `bugprone-string-literal-with-embedded-nul `_, , "medium" `bugprone-suspicious-enum-usage `_, , "high" + `bugprone-suspicious-memory-comparison `_, , "medium" `bugprone-suspicious-memset-usage `_, "Yes", "high" `bugprone-suspicious-missing-comma `_, , "high" `bugprone-suspicious-semicolon `_, "Yes", "high" @@ -298,6 +299,7 @@ `cert-dcl59-cpp `_, `google-build-namespaces `_, , "medium" `cert-err09-cpp `_, `misc-throw-by-value-catch-by-reference `_, , "high" `cert-err61-cpp `_, `misc-throw-by-value-catch-by-reference `_, , "high" + `cert-exp42-c `_, `bugprone-suspicious-memory-comparison `_, , "medium" `cert-fio38-c `_, `misc-non-copyable-objects `_, , "high" `cert-msc30-c `_, `cert-msc50-cpp `_, , "low" `cert-msc32-c `_, `cert-msc51-cpp `_, , "medium" Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp @@ -0,0 +1,352 @@ +// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t + +typedef unsigned long long size_t; +int memcmp(const void *lhs, const void *rhs, size_t count); + +namespace std { +int memcmp(const void *lhs, const void *rhs, size_t count); +} + +namespace sei_cert_example_exp42_c { +struct s { + char c; + int i; + char buffer[13]; +}; + +void noncompliant(const struct s *left, const struct s *right) { + if ((left && right) && (0 == memcmp(left, right, sizeof(struct s)))) { + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: comparing padding bytes + } +} + +void compliant(const struct s *left, const struct s *right) { + if ((left && right) && (left->c == right->c) && (left->i == right->i) && + (0 == memcmp(left->buffer, right->buffer, 13))) { + } +} +} // namespace sei_cert_example_exp42_c + +namespace sei_cert_example_exp42_c_ex1 { +#pragma pack(push, 1) +struct s { + char c; + int i; + char buffer[13]; +}; +#pragma pack(pop) + +void compare(const struct s *left, const struct s *right) { + if ((left && right) && (0 == memcmp(left, right, sizeof(struct s)))) { + // no-warning + } +} +} // namespace sei_cert_example_exp42_c_ex1 + +namespace sei_cert_example_oop57_cpp { +class C { + int i; + +public: + virtual void f(); +}; + +void f(C &c1, C &c2) { + if (!std::memcmp(&c1, &c2, sizeof(C))) { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation + // of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider + // using a comparison operator instead + } +} +} // namespace sei_cert_example_oop57_cpp + +namespace trailing_padding { +struct S { + int i; + char c; +}; + +void test() { + S a, b; + memcmp(&a, &b, sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(int)); + memcmp(&a, &b, sizeof(int) + sizeof(char)); + memcmp(&a, &b, 2 * sizeof(int)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace trailing_padding + +namespace inner_padding { +struct S { + char c; + int i; +}; + +void test() { + S a, b; + memcmp(&a, &b, sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(char) + sizeof(int)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(char)); + memcmp(&a, &b, 2 * sizeof(char)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace inner_padding + +namespace bitfield { +struct S { + int x : 10; + int y : 6; +}; + +void test() { + S a, b; + memcmp(&a, &b, sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, 2); // no-warning: no padding in first 2 bytes +} +} // namespace bitfield + +namespace bitfield_2 { +struct S { + int x : 10; + int y : 7; +}; + +void test() { + S a, b; + memcmp(&a, &b, sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, 2); + memcmp(&a, &b, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace bitfield_2 + +namespace bitfield_3 { +struct S { + int x : 2; + int : 0; + int y : 6; +}; + +void test() { + S a, b; + memcmp(&a, &b, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace bitfield_3 + +namespace array_no_padding { +struct S { + int x; + int y; +}; + +void test() { + S a[3]; + S b[3]; + memcmp(a, b, 3 * sizeof(S)); +} +} // namespace array_no_padding + +namespace array_with_padding { +struct S { + int x; + char y; +}; + +void test() { + S a[3]; + S b[3]; + memcmp(a, b, 3 * sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace array_with_padding + +namespace unknown_count { +struct S { + char c; + int i; +}; + +void test(size_t c) { + S a; + S b; + memcmp(&a, &b, c); // no-warning: unknown count value +} +} // namespace unknown_count + +namespace predeclared_type { +struct S; + +void test(const S *lhs, const S *rhs) { + memcmp(lhs, rhs, 1); // no-warning: predeclared type +} +} // namespace predeclared_type + +namespace padding_after_union { +struct S { + union { + char c; + short s; + } x; + + int y; +}; + +void test() { + S a; + S b; + memcmp(&a, &b, sizeof(short)); + memcmp(&a, &b, sizeof(int)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace padding_after_union + +namespace union_no_padding { +struct S { + union { + int a; + char b; + } x; + + int y; +}; + +void test() { + S a; + S b; + memcmp(&a, &b, 2 * sizeof(int)); + memcmp(&a, &b, sizeof(S)); +} +} // namespace union_no_padding + +namespace padding_in_nested_struct { +struct S { + int a; + char b; +}; +struct T { + S x; + char y; +}; + +void test() { + T a, b; + memcmp(&a, &b, sizeof(int) + sizeof(char)); + memcmp(&a, &b, sizeof(int) + 2 * sizeof(char)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(S)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(T)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace padding_in_nested_struct + +namespace padding_after_nested_struct { +struct S { + char a; +}; +struct T { + S x; + int y; +}; + +void test() { + T a, b; + memcmp(&a, &b, sizeof(char)); + memcmp(&a, &b, sizeof(S)); + memcmp(&a, &b, sizeof(T)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} +} // namespace padding_after_nested_struct + +namespace padding_in_base { +class Base { + char c; + int i; +}; + +class Derived : public Base {}; + +class Derived2 : public Derived {}; + +void testDerived() { + Derived a, b; + memcmp(&a, &b, sizeof(char)); + memcmp(&a, &b, sizeof(Base)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(Derived)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} + +void testDerived2() { + Derived2 a, b; + memcmp(&a, &b, sizeof(char)); + memcmp(&a, &b, sizeof(Base)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes + memcmp(&a, &b, sizeof(Derived2)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes +} + +} // namespace padding_in_base + +namespace no_padding_in_base { +class Base { + int a, b; +}; + +class Derived : public Base {}; + +class Derived2 : public Derived {}; + +void testDerived() { + Derived a, b; + memcmp(&a, &b, sizeof(Base)); + memcmp(&a, &b, sizeof(Derived)); +} + +void testDerived2() { + Derived2 a, b; + memcmp(&a, &b, sizeof(char)); + memcmp(&a, &b, sizeof(Base)); + memcmp(&a, &b, sizeof(Derived2)); +} +} // namespace no_padding_in_base + +namespace non_standard_layout { +class C { +private: + int x; + +public: + int y; +}; + +void test() { + C a, b; + memcmp(&a, &b, sizeof(C)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation + // of non-standard-layout type 'non_standard_layout::C'; consider using a + // comparison operator instead +} + +} // namespace non_standard_layout + +namespace static_ignored { +struct S { + static char c; + int i; +}; + +void test() { + S a, b; + memcmp(&a, &b, sizeof(S)); +} + +} // namespace static_ignored