diff --git a/clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.h b/clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.h @@ -0,0 +1,35 @@ +//===--- AllocationBoolConversionCheck.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_ALLOCATIONBOOLCONVERSIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ALLOCATIONBOOLCONVERSIONCHECK_H + +#include "../ClangTidyCheck.h" +#include + +namespace clang::tidy::bugprone { + +/// Detects cases where the result of a resource allocation is used as a +/// ``bool``. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/allocation-bool-conversion.html +class AllocationBoolConversionCheck : public ClangTidyCheck { +public: + AllocationBoolConversionCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + std::vector AllocationFunctions; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ALLOCATIONBOOLCONVERSIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp @@ -0,0 +1,64 @@ +//===--- AllocationBoolConversionCheck.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 "AllocationBoolConversionCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +AllocationBoolConversionCheck::AllocationBoolConversionCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllocationFunctions(utils::options::parseStringList( + Options.get("AllocationFunctions", + "malloc;calloc;realloc;strdup;fopen;fdopen;freopen;" + "opendir;fdopendir;popen;mmap;allocate"))) {} + +void AllocationBoolConversionCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllocationFunctions", + utils::options::serializeStringList(AllocationFunctions)); +} + +void AllocationBoolConversionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + castExpr( + hasCastKind(CK_PointerToBoolean), + hasSourceExpression(ignoringImplicit(anyOf( + cxxNewExpr(), + callExpr(callee(functionDecl(anyOf(hasAnyOverloadedOperatorName( + "new", "new[]"), + matchers::matchesAnyListedName( + AllocationFunctions))) + .bind("func"))))))) + .bind("cast"), + this); +} + +void AllocationBoolConversionCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedExpr = Result.Nodes.getNodeAs("cast"); + + if (const auto *Function = Result.Nodes.getNodeAs("func")) { + diag(MatchedExpr->getExprLoc(), + "result of the %0 call is being used as a boolean value, which " + "may lead to unintended behavior or resource leaks") + << Function; + } else { + diag(MatchedExpr->getExprLoc(), + "result of the 'new' expression is being used as a boolean value, " + "which may lead to unintended behavior or resource leaks"); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "../cppcoreguidelines/NarrowingConversionsCheck.h" +#include "AllocationBoolConversionCheck.h" #include "ArgumentCommentCheck.h" #include "AssertSideEffectCheck.h" #include "AssignmentInIfConditionCheck.h" @@ -91,6 +92,8 @@ class BugproneModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "bugprone-allocation-bool-conversion"); CheckFactories.registerCheck( "bugprone-argument-comment"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -4,6 +4,7 @@ ) add_clang_library(clangTidyBugproneModule + AllocationBoolConversionCheck.cpp ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp AssignmentInIfConditionCheck.cpp @@ -22,7 +23,6 @@ ForwardingReferenceOverloadCheck.cpp ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp - SwitchMissingDefaultCaseCheck.cpp IncDecInConditionsCheck.cpp IncorrectRoundingsCheck.cpp InfiniteLoopCheck.cpp @@ -66,6 +66,7 @@ SuspiciousSemicolonCheck.cpp SuspiciousStringCompareCheck.cpp SwappedArgumentsCheck.cpp + SwitchMissingDefaultCaseCheck.cpp TerminatingContinueCheck.cpp ThrowKeywordMissingCheck.cpp TooSmallLoopVariableCheck.cpp diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -116,6 +116,11 @@ New checks ^^^^^^^^^^ +- New :doc:`bugprone-allocation-bool-conversion + ` check. + + Detects cases where the result of a resource allocation is used as a ``bool``. + - New :doc:`bugprone-inc-dec-in-conditions ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/allocation-bool-conversion.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/allocation-bool-conversion.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/allocation-bool-conversion.rst @@ -0,0 +1,56 @@ +.. title:: clang-tidy - bugprone-allocation-bool-conversion + +bugprone-allocation-bool-conversion +=================================== + +Detects cases where the result of a resource allocation is used as a +``bool``. + +Functions like ``new``, ``malloc``, ``fopen``, etc., dynamically allocate memory +or resources and return pointers to the newly created objects. However, using +these pointers directly as boolean values, either through implicit or explicit +conversion, a hidden problem emerges. The crux of the issue lies in the fact +that any non-null pointer implicitly converts to ``true``, masking potential +errors and making the code difficult to comprehend. Worse yet, it may trigger +memory leaks, as dynamically allocated resources remain inaccessible, never to +be released by the program. + +Example: + +.. code-block:: c++ + + #include + + bool processResource(bool resourceFlag) { + if (resourceFlag) { + std::cout << "Resource processing successful!" << std::endl; + return true; + } else { + std::cout << "Resource processing failed!" << std::endl; + return false; + } + } + + int main() { + // Implicit conversion of int* to bool + processResource(new int()); + return 0; + } + +In this example, we pass the result of the ``new int()`` expression directly to +the ``processResource`` function as its argument. Since the pointer returned by +``new`` is non-null, it is implicitly converted to ``true``, leading to +incorrect behavior in the ``processResource`` function. + +Check does not offer any auto-fixes. + +Options +------- + +.. option:: AllocationFunctions + + Custom functions considered as allocators can be specified using a + semicolon-separated list of (fully qualified) function names or regular + expressions matched against the called function names. Default value: + `malloc;calloc;realloc;strdup;fopen;fdopen;freopen;opendir;fdopendir;popen; + mmap;allocate`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -76,6 +76,7 @@ `android-cloexec-socket `_, "Yes" `android-comparison-in-temp-failure-retry `_, `boost-use-to-string `_, "Yes" + `bugprone-allocation-bool-conversion `_, `bugprone-argument-comment `_, "Yes" `bugprone-assert-side-effect `_, `bugprone-assignment-in-if-condition `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp @@ -0,0 +1,99 @@ +// RUN: %check_clang_tidy %s bugprone-allocation-bool-conversion %t -- -config="{CheckOptions: { bugprone-allocation-bool-conversion.portability-restrict-system-includes.AllocationFunctions: 'malloc;allocate;custom' }}" + +typedef __SIZE_TYPE__ size_t; + +void takeBool(bool); +void* operator new(size_t count); +void *malloc(size_t size); + +template +struct Allocator { + typedef T* pointer; + pointer allocate(size_t n, const void* hint = 0); +}; + +void* custom(); +void* negative(); + +static Allocator allocator; + +void testImplicit() { + takeBool(negative()); + + takeBool(new int); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(new bool); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(operator new(10)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'operator new' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(malloc(10)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'malloc' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(allocator.allocate(1U)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'allocate' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + + takeBool(custom()); + + bool value; + + value = negative(); + + value = new int; + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = new bool; + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = operator new(10); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'operator new' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = malloc(10); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'malloc' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = allocator.allocate(1U); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'allocate' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = custom(); +} + +void testExplicit() { + takeBool(static_cast(negative())); + + takeBool(static_cast(new int)); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(static_cast(new bool)); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(static_cast(operator new(10))); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'operator new' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(static_cast(malloc(10))); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'malloc' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(static_cast(allocator.allocate(1U))); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'allocate' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(static_cast(custom())); +} + +void testNegation() { + takeBool(!negative()); + + takeBool(!new bool); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(!new int); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(!operator new(10)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'operator new' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(!malloc(10)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'malloc' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(!allocator.allocate(1U)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'allocate' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(!custom()); + + bool value; + + value = !negative(); + + value = !new int; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = !new bool; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'new' expression is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = !operator new(10); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'operator new' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = !malloc(10); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'malloc' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = !allocator.allocate(1U); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'allocate' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = !custom(); +}