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,36 @@ +//===--- 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 PointerReturningAllocatorsConfig; + std::vector IntegerReturningAllocatorsConfig; +}; + +} // 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,122 @@ +//===--- 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" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +static const std::array PointerReturningAllocators{ + {"malloc", + "calloc", + "realloc", + "aligned_alloc", + "allocate", + "fopen", + "fdopen", + "freopen", + "popen", + "tmpfile", + "::opendir", + "::fdopendir", + "::mmap", + "::reallocf", + "::strdup", + "::wcsdup", + "::strndup", + "::realpath", + "::tempnam", + "::canonicalize_file_name", + "::dbopen", + "::fmemopen", + "::open_memstream", + "::open_wmemstream", + "::get_current_dir_name", + "memalloc", + "memcalloc", + "memrealloc", + "::mpool_open", + "::posix_memalign", + "::memalign", + "::valloc", + "::pvalloc"}}; + +static const std::array IntegerReturningAllocators{ + {"::open", "::openat", "::creat", "::dup", "::dup2", "::dup3", "::socket", + "::accept", "::pipe", "::pipe2", "::mkfifo", "::mkfifoat", "::mkstemp", + "::mkostemp", "::mkstemps", "::mkostemps"}}; + +AllocationBoolConversionCheck::AllocationBoolConversionCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + PointerReturningAllocatorsConfig(utils::options::parseStringList( + Options.get("PointerReturningAllocators", ""))), + IntegerReturningAllocatorsConfig(utils::options::parseStringList( + Options.get("IntegerReturningAllocators", ""))) {} + +void AllocationBoolConversionCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store( + Opts, "PointerReturningAllocators", + utils::options::serializeStringList(PointerReturningAllocatorsConfig)); + Options.store( + Opts, "IntegerReturningAllocators", + utils::options::serializeStringList(IntegerReturningAllocatorsConfig)); +} + +void AllocationBoolConversionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + castExpr(hasCastKind(CK_PointerToBoolean), + hasSourceExpression(ignoringImplicit(anyOf( + cxxNewExpr(), + callExpr(callee( + functionDecl( + anyOf(hasAnyOverloadedOperatorName("new", "new[]"), + matchers::matchesAnyListedName( + PointerReturningAllocators), + matchers::matchesAnyListedName( + PointerReturningAllocatorsConfig))) + .bind("func"))))))) + .bind("cast"), + this); + + Finder->addMatcher( + castExpr(hasCastKind(CK_IntegralToBoolean), + hasSourceExpression(ignoringImplicit(callExpr(callee( + functionDecl(anyOf(matchers::matchesAnyListedName( + IntegerReturningAllocators), + matchers::matchesAnyListedName( + IntegerReturningAllocatorsConfig))) + .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,82 @@ +.. 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``. + +Expressions like ``new`` and ``new[]`` and functions like ``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. Similar issues exist with ``open``, ``socket``, ``accept``, etc., +those functions returns handle to resources in form of integer that can be +silently convert to boolean. 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. + +Predefined list of pointer-returning allocator functions: ``malloc``, +``calloc``, ``realloc``, ``aligned_alloc``, ``allocate``, ``fopen``, ``fdopen``, +``freopen``, ``popen``, ``tmpfile``, ``::opendir``, ``::fdopendir``, ``::mmap``, +``::reallocf``, ``::strdup``, ``::wcsdup``, ``::strndup``, ``::realpath``, +``::tempnam``, ``::canonicalize_file_name``, ``::dbopen``, ``::fmemopen``, +``::open_memstream``, ``::open_wmemstream``, ``::get_current_dir_name``, +``memalloc``, ``memcalloc``, ``memrealloc``, ``::mpool_open``, +``::posix_memalign``, ``::memalign``, ``::valloc``, ``::pvalloc"``. + +Predefined list of integer-returning allocator functions: ``::open``, +``::openat``, ``::creat``, ``::dup``, ``::dup2``, ``::dup3``, ``::socket``, +``::accept``, ``::pipe``, ``::pipe2``, ``::mkfifo``, ``::mkfifoat``, +``::mkstemp``, ``::mkostemp``, ``::mkstemps``, ``::mkostemps``. + +The options `PointerReturningAllocators` and `IntegerReturningAllocators` allow +for an extension of those lists. + +Check does not offer any auto-fixes. + +Options +------- + +.. option:: PointerReturningAllocators + + List of additional 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: empty string. + +.. option:: IntegerReturningAllocators + + List of additional 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: empty string. 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,133 @@ +// RUN: %check_clang_tidy %s bugprone-allocation-bool-conversion %t -- -config="{CheckOptions: { \ +// RUN: bugprone-allocation-bool-conversion.PointerReturningAllocators: 'ptr_custom',\ +// RUN: bugprone-allocation-bool-conversion.IntegerReturningAllocators: 'int_custom'}}" + +typedef __SIZE_TYPE__ size_t; + +void takeBool(bool); +void* operator new(size_t count); +void *malloc(size_t size); +int open(const char *pathname, int flags); + +template +struct Allocator { + typedef T* pointer; + pointer allocate(size_t n, const void* hint = 0); +}; + +void* ptr_custom(); +int int_custom(); +void* negative(); +int negativeInt(); + +static Allocator allocator; + +void testImplicit() { + takeBool(negative()); + takeBool(negativeInt()); + + 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(open("file", 0)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'open' 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(ptr_custom()); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'ptr_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(int_custom()); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'int_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + + bool value; + + value = negative(); + value = negativeInt(); + + 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 = open("file", 0); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'open' 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 = ptr_custom(); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'ptr_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = int_custom(); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of the 'int_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] +} + +void testExplicit() { + takeBool(static_cast(negative())); + takeBool(static_cast(negativeInt())); + + 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(open("file", 0))); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'open' 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(ptr_custom())); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'ptr_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(static_cast(int_custom())); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: result of the 'int_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] +} + +void testNegation() { + takeBool(!negative()); + takeBool(!negativeInt()); + + 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(!open("file", 0)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'open' 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(!ptr_custom()); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'ptr_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + takeBool(!int_custom()); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of the 'int_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + + bool value; + + value = !negative(); + value = !negativeInt(); + + 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 = !open("file", 0); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'open' 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 = !ptr_custom(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'ptr_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] + value = !int_custom(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of the 'int_custom' call is being used as a boolean value, which may lead to unintended behavior or resource leaks [bugprone-allocation-bool-conversion] +}