[clang-tidy] Add bugprone-allocation-bool-conversion

[PiotrZSL]

Summary

Detects cases where the result of a resource allocation is used as a bool.

Fixes: #64461

[Eugene.Zelenko]

Shouldn't C-style allocation be checked too? Same for custom allocators via configuration option.

[Eugene.Zelenko]

Same for file, threads and other resources allocation.

[PiotrZSL]

In D157188#4563105, @Eugene.Zelenko wrote:

Shouldn't C-style allocation be checked too? Same for custom allocators via configuration option.

In theory it could, but in such case name of check would need to change...

[Eugene.Zelenko]

In D157188#4563139, @PiotrZSL wrote:

In D157188#4563105, @Eugene.Zelenko wrote:

Shouldn't C-style allocation be checked too? Same for custom allocators via configuration option.

In theory it could, but in such case name of check would need to change...

Something like bugprone-resource-bool-conversion should be generic enough.

[chandlerc]

In D157188#4563143, @Eugene.Zelenko wrote:

In D157188#4563139, @PiotrZSL wrote:

In D157188#4563105, @Eugene.Zelenko wrote:

Shouldn't C-style allocation be checked too? Same for custom allocators via configuration option.

In theory it could, but in such case name of check would need to change...

Something like bugprone-resource-bool-conversion should be generic enough.

Maybe bugprone-allocation-bool-conversion?

[PiotrZSL]

Change check anme, add configuration option.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp

26

POSIX open, openat, creat, pthread_create should be added too.

61

Should be merged together.

[PiotrZSL]

Formating + fix tests

clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp

26

Those return integers, not pointers. Check is for pointers only, but probably could be extended to integers if we go into this direction.

[PiotrZSL]

TODO: Fix CI, Add support for allocators that return 'integer', hardcode most of functions, change config into AdditionalAllocationFunctions

[PiotrZSL]

Rebase, add support for integer returnign functions, fixes in tests.

clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/bugprone/AllocationBoolConversionCheck.cpp

+//===--- 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

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 //===--- BugproneTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../cppcoreguidelines/NarrowingConversionsCheck.h"
+#include "AllocationBoolConversionCheck.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignmentInIfConditionCheck.h"
 #include "BadSignalToKillThreadCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"

 #include "VirtualNearMissCheck.h"
 
 namespace clang::tidy {
 namespace bugprone {
 
 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>(
         "bugprone-assert-side-effect");
     CheckFactories.registerCheck<AssignmentInIfConditionCheck>(
         "bugprone-assignment-in-if-condition");
     CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
         "bugprone-bad-signal-to-kill-thread");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   FrontendOpenMP
   )
 
 add_clang_library(clangTidyBugproneModule
+  AllocationBoolConversionCheck.cpp
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignmentInIfConditionCheck.cpp
   BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
   EmptyCatchCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
-  SwitchMissingDefaultCaseCheck.cpp
   IncDecInConditionsCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
add_clang_library(clangTidyBugproneModule
   SuspiciousIncludeCheck.cpp
   SuspiciousMemoryComparisonCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousReallocUsageCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
+  SwitchMissingDefaultCaseCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
   UncheckedOptionalAccessCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnhandledExceptionAtNewCheck.cpp
   UnhandledSelfAssignmentCheck.cpp

clang-tools-extra/docs/ReleaseNotes.rst

 - Remove configuration option `AnalyzeTemporaryDestructors`, which was deprecated since
   :program:`clang-tidy` 16.
 
 - Improved `--dump-config` to print check options in alphabetical order.
 
 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.
 
   Detects when a variable is both incremented/decremented and referenced inside
   a complex condition and suggests moving them outside to avoid ambiguity in
   the variable's value.
 
 - New :doc:`bugprone-multi-level-implicit-pointer-conversion

clang-tools-extra/docs/clang-tidy/checks/bugprone/allocation-bool-conversion.rst

+.. 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`.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `android-cloexec-inotify-init1 <android/cloexec-inotify-init1.html>`_, "Yes"
    `android-cloexec-memfd-create <android/cloexec-memfd-create.html>`_, "Yes"
    `android-cloexec-open <android/cloexec-open.html>`_, "Yes"
    `android-cloexec-pipe <android/cloexec-pipe.html>`_, "Yes"
    `android-cloexec-pipe2 <android/cloexec-pipe2.html>`_, "Yes"
    `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>`_,
    `bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread.html>`_,
    `bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion.html>`_, "Yes"
    `bugprone-branch-clone <bugprone/branch-clone.html>`_,
    `bugprone-copy-constructor-init <bugprone/copy-constructor-init.html>`_, "Yes"
    `bugprone-dangling-handle <bugprone/dangling-handle.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/allocation-bool-conversion.cpp

+// 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<typename T>
+struct Allocator {
+  typedef T* pointer;
+  pointer allocate(size_t 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();
+}