PiotrZSL updated this revision to Diff 547523. PiotrZSL retitled this revision from "[clang-tidy] Add bugprone-new-bool-conversion" to "[clang-tidy] Add bugprone-allocation-bool-conversion". PiotrZSL edited the summary of this revision. PiotrZSL added a comment.
Change check anme, add configuration option. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157188/new/ https://reviews.llvm.org/D157188 Files: clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.h clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/bugprone/allocation-bool-conversion.rst clang-tools-extra/docs/clang-tidy/checks/list.rst clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp @@ -0,0 +1,97 @@ +// RUN: %check_clang_tidy %s bugprone-allocation-bool-conversion %t -- -config="{CheckOptions: { bugprone-allocation-bool-conversion.portability-restrict-system-includes.AllocationFunctions: 'malloc;allocate;custom' }}" + +void takeBool(bool); +void* operator new(unsigned long count); +void *malloc(unsigned long size); + +template<typename T> +struct Allocator { + typedef T* pointer; + pointer allocate(unsigned long n, const void* hint = 0); +}; + +void* custom(); +void* negative(); + +static Allocator<int> 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<bool>(negative())); + + takeBool(static_cast<bool>(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<bool>(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<bool>(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<bool>(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<bool>(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<bool>(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(); +} 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 @@ `android-cloexec-socket <android/cloexec-socket.html>`_, "Yes" `android-comparison-in-temp-failure-retry <android/comparison-in-temp-failure-retry.html>`_, `boost-use-to-string <boost/use-to-string.html>`_, "Yes" + `bugprone-allocation-bool-conversion <bugprone/allocation-bool-conversion.html>`_, `bugprone-argument-comment <bugprone/argument-comment.html>`_, "Yes" `bugprone-assert-side-effect <bugprone/assert-side-effect.html>`_, `bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition.html>`_, Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/allocation-bool-conversion.rst =================================================================== --- /dev/null +++ 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 <iostream> + + 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`. Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -116,6 +116,11 @@ New checks ^^^^^^^^^^ +- New :doc:`bugprone-allocation-bool-conversion + <clang-tidy/checks/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 <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check. 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 @@ -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 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 @@ -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<AllocationBoolConversionCheck>( + "bugprone-allocation-bool-conversion"); CheckFactories.registerCheck<ArgumentCommentCheck>( "bugprone-argument-comment"); CheckFactories.registerCheck<AssertSideEffectCheck>( Index: clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.h =================================================================== --- /dev/null +++ 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 <vector> + +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<llvm::StringRef> AllocationFunctions; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ALLOCATIONBOOLCONVERSIONCHECK_H Index: clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp @@ -0,0 +1,65 @@ +//===--- 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<CastExpr>("cast"); + + if (const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits