diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index b780a85bdf3fe..0b2b89087f29c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -53,6 +53,7 @@ #include "NonZeroEnumToBoolConversionCheck.h" #include "NondeterministicPointerIterationOrderCheck.h" #include "NotNullTerminatedResultCheck.h" +#include "NullCheckAfterDereferenceCheck.h" #include "OptionalValueConversionCheck.h" #include "ParentVirtualCallCheck.h" #include "PointerArithmeticOnPolymorphicObjectCheck.h" @@ -204,6 +205,8 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck("bugprone-posix-return"); CheckFactories.registerCheck( "bugprone-reserved-identifier"); + CheckFactories.registerCheck( + "bugprone-null-check-after-dereference"); CheckFactories.registerCheck( "bugprone-shared-ptr-array-mismatch"); CheckFactories.registerCheck("bugprone-signal-handler"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e310ea9c94543..8081d21078a10 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -54,6 +54,7 @@ add_clang_library(clangTidyBugproneModule STATIC NonZeroEnumToBoolConversionCheck.cpp NondeterministicPointerIterationOrderCheck.cpp NotNullTerminatedResultCheck.cpp + NullCheckAfterDereferenceCheck.cpp OptionalValueConversionCheck.cpp ParentVirtualCallCheck.cpp PointerArithmeticOnPolymorphicObjectCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp new file mode 100644 index 0000000000000..6ea8e174b7149 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp @@ -0,0 +1,152 @@ +//===--- NullCheckAfterDereferenceCheck.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 "NullCheckAfterDereferenceCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/DeclCXX.h" +#include "clang/AST/DeclTemplate.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h" +#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" +#include "clang/Analysis/FlowSensitive/DataflowLattice.h" +#include "clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h" +#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h" +#include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/Any.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/Support/Error.h" +#include +#include +#include + +namespace clang::tidy::bugprone { + +using ast_matchers::MatchFinder; +using dataflow::NullCheckAfterDereferenceDiagnoser; +using dataflow::NullPointerAnalysisModel; +using Diagnoser = NullCheckAfterDereferenceDiagnoser; + +static constexpr llvm::StringLiteral FuncID("fun"); + +struct ExpandedResult { + Diagnoser::DiagnosticEntry Entry; + std::optional DerefLoc; +}; + +using ExpandedResultType = llvm::SmallVector; + +static std::optional +analyzeFunction(const FunctionDecl &FuncDecl) { + using dataflow::AdornedCFG; + using dataflow::DataflowAnalysisState; + using llvm::Expected; + + ASTContext &ASTCtx = FuncDecl.getASTContext(); + + if (FuncDecl.getBody() == nullptr) { + return std::nullopt; + } + + Expected Context = + AdornedCFG::build(FuncDecl, *FuncDecl.getBody(), ASTCtx); + if (!Context) + return std::nullopt; + + dataflow::DataflowAnalysisContext AnalysisContext( + std::make_unique()); + dataflow::Environment Env(AnalysisContext, FuncDecl); + NullPointerAnalysisModel Analysis(ASTCtx); + Diagnoser Diagnoser; + + Diagnoser::ResultType Diagnostics; + + if (llvm::Error E = dataflow::diagnoseFunction( + FuncDecl, ASTCtx, Diagnoser).moveInto(Diagnostics)) { + llvm::dbgs() << "Dataflow analysis failed: " << llvm::toString(std::move(E)) + << ".\n"; + return std::nullopt; + } + + ExpandedResultType ExpandedDiagnostics; + + llvm::transform(Diagnostics, + std::back_inserter(ExpandedDiagnostics), + [&](Diagnoser::DiagnosticEntry Entry) -> ExpandedResult { + if (auto Val = Diagnoser.WarningLocToVal[Entry.Location]; + auto DerefExpr = Diagnoser.ValToDerefLoc[Val]) { + return {Entry, DerefExpr->getBeginLoc()}; + } + + return {Entry, std::nullopt}; + }); + + return ExpandedDiagnostics; +} + +void NullCheckAfterDereferenceCheck::registerMatchers(MatchFinder *Finder) { + using namespace ast_matchers; + + auto containsPointerValue = + hasDescendant(NullPointerAnalysisModel::ptrValueMatcher()); + Finder->addMatcher( + decl(anyOf(functionDecl(unless(isExpansionInSystemHeader()), + // FIXME: Remove the filter below when lambdas are + // well supported by the check. + unless(hasDeclContext(cxxRecordDecl(isLambda()))), + hasBody(containsPointerValue)), + cxxConstructorDecl( + unless(hasDeclContext(cxxRecordDecl(isLambda()))), + hasAnyConstructorInitializer( + withInitializer(containsPointerValue))))) + .bind(FuncID), + this); +} + +void NullCheckAfterDereferenceCheck::check( + const MatchFinder::MatchResult &Result) { + if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred()) + return; + + const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID); + assert(FuncDecl && "invalid FuncDecl matcher"); + if (FuncDecl->isTemplated()) + return; + + if (const auto Diagnostics = analyzeFunction(*FuncDecl)) { + for (const auto [Entry, DerefLoc] : *Diagnostics) { + const auto [WarningLoc, Type] = Entry; + + switch (Type) { + case Diagnoser::DiagnosticType::CheckAfterDeref: + diag(WarningLoc, "pointer value is checked even though " + "it cannot be null at this point"); + + if (DerefLoc) { + diag(*DerefLoc, + "one of the locations where the pointer's value cannot be null", + DiagnosticIDs::Note); + } + break; + case Diagnoser::DiagnosticType::CheckWhenNull: + diag(WarningLoc, + "pointer value is checked but it can only be null at this point"); + + if (DerefLoc) { + diag(*DerefLoc, + "one of the locations where the pointer's value can only be null", + DiagnosticIDs::Note); + } + break; + } + } + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h new file mode 100644 index 0000000000000..e5ac27e79deb1 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h @@ -0,0 +1,37 @@ +//===--- NullCheckAfterDereferenceCheck.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_NULLCHECKAFTERDEREFERENCECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NULLCHECKAFTERDEREFERENCECHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +namespace clang::tidy::bugprone { + +/// Finds checks for pointer nullability after a pointer has already been +/// dereferenced, using the data-flow framework. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/null-check-after-dereference.html +class NullCheckAfterDereferenceCheck : public ClangTidyCheck { +public: + NullCheckAfterDereferenceCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + + // The data-flow framework does not support C because of AST differences. + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NULLCHECKAFTERDEREFERENCECHECK_H diff --git a/clang-tools-extra/clangd/TidyProvider.cpp b/clang-tools-extra/clangd/TidyProvider.cpp index 1d79a7a7399ec..ec933b3789d77 100644 --- a/clang-tools-extra/clangd/TidyProvider.cpp +++ b/clang-tools-extra/clangd/TidyProvider.cpp @@ -220,9 +220,10 @@ TidyProvider disableUnusableChecks(llvm::ArrayRef ExtraBadChecks) { "-bugprone-use-after-move", // Alias for bugprone-use-after-move. "-hicpp-invalid-access-moved", - // Check uses dataflow analysis, which might hang/crash unexpectedly on + // Checks use dataflow analysis, which might hang/crash unexpectedly on // incomplete code. - "-bugprone-unchecked-optional-access"); + "-bugprone-unchecked-optional-access", + "-bugprone-null-check-after-dereference"); size_t Size = BadChecks.size(); for (const std::string &Str : ExtraBadChecks) { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6cb8d572d3a78..303498803b363 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -103,6 +103,12 @@ New checks Finds lambda captures that capture the ``this`` pointer and store it as class members without handle the copy and move constructors and the assignments. +- New :doc:`bugprone-null-check-after-dereference + ` check. + + Identifies redundant pointer null-checks, by finding cases where + the pointer cannot be null at the location of the null-check. + - New :doc:`bugprone-unintended-char-ostream-output ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst new file mode 100644 index 0000000000000..70b8311af947a --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst @@ -0,0 +1,163 @@ +.. title:: clang-tidy - bugprone-null-check-after-dereference + +bugprone-null-check-after-dereference +===================================== + +.. note:: + + This check uses a flow-sensitive static analysis to produce its + results. Therefore, it may be more resource intensive (RAM, CPU) than the + average Clang-tidy check. + +Identifies redundant pointer null-checks, by finding cases where the pointer +cannot be null at the location of the null-check. + +Redundant null-checks can signal faulty assumptions about the current value of +a pointer at different points in the program. Either the null-check is +redundant, or there could be a null-pointer dereference earlier in the program. + +.. code-block:: c++ + + int f(int *ptr) { + *ptr = 20; // note: one of the locations where the pointer's value cannot be null + // ... + if (ptr) { // bugprone: pointer is checked even though it cannot be null at this point + return *ptr; + } + return 0; + } + +Supported pointer operations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Pointer null-checks +------------------- + +The check currently supports null-checks on pointers that use +``operator bool``, such as when being used as the condition +for an ``if`` statement. It also supports comparisons such as ``!= nullptr``, and +``== other_ptr``. + +.. code-block:: c++ + + int f(int *ptr) { + if (ptr) { + if (ptr) { // bugprone: pointer is re-checked after its null-ness is already checked. + return *ptr; + } + + return ptr ? *ptr : 0; // bugprone: pointer is re-checked after its null-ness is already checked. + } + return 0; + } + +Pointer dereferences +-------------------- + +Pointer star- and arrow-dereferences are supported. + +.. code-block:: c++ + + struct S { + int val; + }; + + void f(int *ptr, S *wrapper) { + *ptr = 20; + wrapper->val = 15; + } + +Null-pointer and other value assignments +---------------------------------------- + +The check supports assigning various values to pointers, making them *null* +or *non-null*. The check also supports passing pointers of a pointer to +external functions. + +.. code-block:: c++ + + extern int *external(); + extern void refresh(int **ptr_ptr); + + int f() { + int *ptr_null = nullptr; + if (ptr_null) { // bugprone: pointer is checked where it cannot be non-null. + return *ptr_null; + } + + int *ptr = external(); + if (ptr) { // safe: external() could return either nullable or nonnull pointers. + return *ptr; + } + + int *ptr2 = external(); + *ptr2 = 20; + refresh(&ptr2); + if (ptr2) { // safe: pointer could be changed by refresh(). + return *ptr2; + } + return 0; + } + +Limitations +~~~~~~~~~~~ + +The check only supports C++ due to limitations in the data-flow framework. + +The annotations ``_Nullable`` and ``_Nonnull`` are not supported. + +.. code-block:: c++ + + extern int *_nonnull external_nonnull(); + + int annotations() { + int *ptr = external_nonnull(); + + return ptr ? *ptr : 0; // false-negative: pointer is known to be non-null. + } + +Function calls taking a pointer value as a reference or a pointer-to-pointer are +not supported. + +.. code-block:: c++ + + extern int *external(); + extern void refresh_ref(int *&ptr); + extern void refresh_ptr(int **ptr); + + int extern_ref() { + int *ptr = external(); + *ptr = 20; + + refresh_ref(ptr); + refresh_ptr(&ptr); + + return ptr ? *ptr : 0; // false-positive: pointer could be changed by refresh_ref(). + } + +Note tags are currently appended to a single location, even if all paths ensure +a pointer is not null. + +.. code-block:: c++ + + int branches(int *p, bool b) { + if (b) { + *p = 42; // true-positive: note-tag appended here + } else { + *p = 20; // false-positive: note tag not appended here + } + + return ptr ? *ptr : 0; + } + +Declarations and some other operations are not supported by note tags yet. This +can sometimes result in erroneous note tags being shown instead of the correct +one. + +.. code-block:: c++ + + int note_tags() { + int *ptr = nullptr; // false-negative: note tag not shown + + return ptr ? *ptr : 0; + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp new file mode 100644 index 0000000000000..78709b9210f69 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp @@ -0,0 +1,304 @@ +// RUN: %check_clang_tidy %s bugprone-null-check-after-dereference %t + +struct S { + int a; +}; + +void warning_deref(int *p) { + *p = 42; + + if (p) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point [bugprone-null-check-after-dereference] + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + // FIXME: If there's a direct path, make the error message more precise, ie. remove `one of the locations` + *p += 20; + } +} + +void warning_member(S *q) { + q->a = 42; + + if (q) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + q->a += 20; + } +} + +void negative_warning(int *p) { + *p = 42; + + if (!p) { + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + return; + } + + *p += 20; +} + +void no_warning(int *p, bool b) { + if (b) { + *p = 42; + } + + if (p) { + // no-warning + *p += 20; + } +} + +void equals_nullptr(int *p) { + *p = 42; + + if (p == nullptr) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + return; + } + + if (p != nullptr) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-10]]:3: note: one of the locations where the pointer's value cannot be null + *p += 20; + } + + if (nullptr != p) { + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-16]]:3: note: one of the locations where the pointer's value cannot be null + *p += 20; + } +} + +void equals_other_ptr(int *p, int *q) { + if (q) + return; + + if (p == q) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: pointer value is checked but it can only be null at this point + // CHECK-MESSAGES: :[[@LINE-5]]:7: note: one of the locations where the pointer's value can only be null + return; + } +} + +int else_branch_warning(int *p, bool b) { + if (b) { + *p = 42; + } else { + *p = 20; + } + + if (p) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-7]]:5: note: one of the locations where the pointer's value cannot be null + return 0; + } else { + return *p; + } +} + +int two_branches_warning(int *p, bool b) { + if (b) { + *p = 42; + } + + if (!b) { + *p = 20; + } + + if (p) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null + return 0; + } else { + return *p; + } +} + +int regular_assignment(int *p, int *q) { + *p = 42; + q = p; + + if (q) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-5]]:3: note: one of the locations where the pointer's value cannot be null + return *p; + } else { + return 0; + } +} + +int nullptr_assignment(int *nullptr_param, bool b) { + *nullptr_param = 42; + int *nullptr_assigned; + + if (b) { + nullptr_assigned = nullptr; + } else { + nullptr_assigned = nullptr_param; + } + + if (nullptr_assigned) { + // no-warning + return *nullptr_assigned; + } else { + return 0; + } +} + +extern int *external_fn(); +extern void ref_fn(int *&ptr); +extern void ptr_fn(int **ptr); + +int fncall_reassignment(int *fncall_reassigned) { + *fncall_reassigned = 42; + + fncall_reassigned = external_fn(); + + if (fncall_reassigned) { + // no-warning + *fncall_reassigned = 42; + } + + fncall_reassigned = external_fn(); + + *fncall_reassigned = 42; + + if (fncall_reassigned) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + *fncall_reassigned = 42; + } + + ref_fn(fncall_reassigned); + + if (fncall_reassigned) { + // FIXME: References of a pointer passed to external functions do not invalidate its value + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point + *fncall_reassigned = 42; + } + + *fncall_reassigned = 42; + + ptr_fn(&fncall_reassigned); + + if (fncall_reassigned) { + // no-warning + *fncall_reassigned = 42; + } + + ptr_fn(&fncall_reassigned); + *fncall_reassigned = 42; + + if (fncall_reassigned) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + *fncall_reassigned = 42; + return *fncall_reassigned; + } else { + return 0; + } +} + +int chained_references(int *a, int *b, int *c, int *d, int *e) { + *a = 42; + + if (a) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null + *b = 42; + } + + if (b) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null + *c = 42; + } + + if (c) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null + return *a; + } else { + return 0; + } +} + +int chained_if(int *a) { + if (!a) { + return 0; + } + + if (a) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + *a += 20; + return *a; + } else { + return 0; + } +} + +int double_if(int *a) { + if (a) { + if (a) { + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: pointer value is checked even though it cannot be null at this point + // --CHECK-MESSAGES: :[[@LINE-3]]:5: note: one of the locations where the pointer's value cannot be null + // FIXME: Add warning for branch satements where pointer is not null afterwards + return *a; + } else { + return 0; + } + } + + return 0; +} + +int while_loop(int *p, volatile bool *b) { + while (true) { + if (*b) { + *p = 42; + break; + } + } + + if (p) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-7]]:7: note: one of the locations where the pointer's value cannot be null + *p = 42; + return *p; + } else { + return 0; + } +} + +int ternary_op(int *p, int k) { + *p = 42; + + return p ? *p : k; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: pointer value is checked even though it cannot be null at this point + // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null +} + +// In an earlier version, the check would crash on C++17 structured bindings. +int cxx17_crash(int *p) { + *p = 42; + + int arr[2] = {1, 2}; + auto [a, b] = arr; + + return 0; +} + +// In an earlier version, the check would crash when encountering anonymous lambdas. +void lambda_crash(int *p) { + auto f = [p](){ *p = 42; }; + f(); +} + +int note_tags() { + // FIXME: Note tags are not appended to declarations + int *ptr = nullptr; + + return ptr ? *ptr : 0; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: pointer value is checked but it can only be null at this point +} diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h new file mode 100644 index 0000000000000..af5145aaef190 --- /dev/null +++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h @@ -0,0 +1,102 @@ +//===-- NullPointerAnalysisModel.h ------------------------------*- 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 +// +//===----------------------------------------------------------------------===// +// +// This file defines a generic null-pointer analysis model, used for finding +// pointer null-checks after the pointer has already been dereferenced. +// +// Only a limited set of operations are currently recognized. Notably, pointer +// arithmetic, null-pointer assignments and _nullable/_nonnull attributes are +// missing as of yet. +// +//===----------------------------------------------------------------------===// + +#ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_NULLPOINTERANALYSISMODEL_H +#define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_NULLPOINTERANALYSISMODEL_H + +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/Stmt.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" +#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" +#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" +#include "clang/Analysis/FlowSensitive/DataflowLattice.h" +#include "clang/Analysis/FlowSensitive/MapLattice.h" +#include "clang/Analysis/FlowSensitive/NoopLattice.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/Twine.h" + +namespace clang::dataflow { + +class NullPointerAnalysisModel + : public DataflowAnalysis { +private: + CFGMatchSwitch TransferMatchSwitch; +public: + explicit NullPointerAnalysisModel(ASTContext &Context); + + static NoopLattice initialElement() { return {}; } + + static ast_matchers::StatementMatcher ptrValueMatcher(); + + void transfer(const CFGElement &E, NoopLattice &State, Environment &Env); + + void join(QualType Type, const Value &Val1, const Environment &Env1, + const Value &Val2, const Environment &Env2, Value &MergedVal, + Environment &MergedEnv) override; + + ComparisonResult compare(QualType Type, const Value &Val1, + const Environment &Env1, const Value &Val2, + const Environment &Env2) override; + + std::optional widen(QualType Type, Value &Prev, + const Environment &PrevEnv, Value &Current, + Environment &CurrentEnv) override; +}; + +class NullCheckAfterDereferenceDiagnoser { +public: + struct DiagnoseArgs { + llvm::DenseMap &ValToDerefLoc; + llvm::DenseMap &WarningLocToVal; + const Environment &Env; + }; + + /// Returns source locations for pointers that were checked when known to be + // null, and checked after already dereferenced, respectively. + enum class DiagnosticType { CheckWhenNull, CheckAfterDeref }; + + struct DiagnosticEntry { + SourceLocation Location; + DiagnosticType Type; + }; + using ResultType = llvm::SmallVector; + + // Maps a pointer's Value to a dereference, null-assignment, etc. + // This is used later to construct the Note tag. + llvm::DenseMap ValToDerefLoc; + // Maps Maps a warning's SourceLocation to its relevant Value. + llvm::DenseMap WarningLocToVal; + +private: + CFGMatchSwitch DiagnoseMatchSwitch; + +public: + NullCheckAfterDereferenceDiagnoser(); + + ResultType operator()(const CFGElement &Elt, ASTContext &Ctx, + const TransferStateForDiagnostics &State); +}; + +} // namespace clang::dataflow + +#endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_NULLPOINTERANALYSISMODEL_H diff --git a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt index 89bbe8791eb2c..6d365dabe6ae5 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt +++ b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt @@ -1,5 +1,6 @@ add_clang_library(clangAnalysisFlowSensitiveModels ChromiumCheckModel.cpp + NullPointerAnalysisModel.cpp UncheckedOptionalAccessModel.cpp LINK_LIBS diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp new file mode 100644 index 0000000000000..36907c8fdbbe2 --- /dev/null +++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp @@ -0,0 +1,788 @@ +//===-- NullPointerAnalysisModel.cpp ----------------------------*- 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 +// +//===----------------------------------------------------------------------===// +// +// This file defines a generic null-pointer analysis model, used for finding +// pointer null-checks after the pointer has already been dereferenced. +// +// Only a limited set of operations are currently recognized. Notably, pointer +// arithmetic, null-pointer assignments and _nullable/_nonnull attributes are +// missing as of yet. +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/Stmt.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" +#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" +#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" +#include "clang/Analysis/FlowSensitive/DataflowLattice.h" +#include "clang/Analysis/FlowSensitive/MapLattice.h" +#include "clang/Analysis/FlowSensitive/NoopLattice.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/Twine.h" + +namespace clang::dataflow { + +namespace { +using namespace ast_matchers; +using Diagnoser = NullCheckAfterDereferenceDiagnoser; + +constexpr char kCond[] = "condition"; +constexpr char kVar[] = "var"; +constexpr char kValue[] = "value"; +constexpr char kIsNonnull[] = "is-nonnull"; +constexpr char kIsNull[] = "is-null"; + +enum class SatisfiabilityResult { + // Returned when the value was not initialized yet. + Nullptr, + // Special value that signals that the boolean value can be anything. + // It signals that the underlying formulas are too complex to be calculated + // efficiently. + Top, + // Equivalent to the literal True in the current environment. + True, + // Equivalent to the literal False in the current environment. + False, + // Both True and False values could be produced with an appropriate set of + // conditions. + Unknown +}; + +enum class CompareResult { + Same, + Different, + Top, + Unknown +}; + +using SR = SatisfiabilityResult; +using CR = CompareResult; + +// FIXME: These AST matchers should also be exported via the +// NullPointerAnalysisModel class, for tests +auto ptrWithBinding(llvm::StringRef VarName = kVar) { + return expr(hasType(isAnyPointer())).bind(VarName); +} + +auto derefMatcher() { + return unaryOperator(hasOperatorName("*"), hasUnaryOperand(ptrWithBinding())); +} + +auto arrowMatcher() { + return memberExpr(allOf(isArrow(), hasObjectExpression(ptrWithBinding()))); +} + +auto castExprMatcher() { + return castExpr(hasCastKind(CK_PointerToBoolean), + hasSourceExpression(ptrWithBinding())) + .bind(kCond); +} + +auto nullptrMatcher(llvm::StringRef VarName = kVar) { + return castExpr(hasCastKind(CK_NullToPointer)).bind(VarName); +} + +auto addressofMatcher() { + return unaryOperator(hasOperatorName("&")).bind(kVar); +} + +auto functionCallMatcher() { + return callExpr( + callee(functionDecl(hasAnyParameter(anyOf(hasType(pointerType()), + hasType(referenceType())))))); +} + +auto assignMatcher() { + return binaryOperation(isAssignmentOperator(), hasLHS(ptrWithBinding()), + hasRHS(expr().bind(kValue))); +} + +auto nullCheckExprMatcher() { + return binaryOperator(hasAnyOperatorName("==", "!="), + hasOperands(ptrWithBinding(), nullptrMatcher(kValue))); +} + +// FIXME: When TK_IgnoreUnlessSpelledInSource is removed from ptrWithBinding(), +// this matcher should be merged with nullCheckExprMatcher(). +auto equalExprMatcher() { + return binaryOperator(hasAnyOperatorName("==", "!="), + hasOperands(ptrWithBinding(kVar), + ptrWithBinding(kValue))); +} + +auto anyPointerMatcher() { return expr(hasType(isAnyPointer())).bind(kVar); } + +// Only computes simple comparisons against the literals True and False in the +// environment. Faster, but produces many Unknown values. +SatisfiabilityResult shallowComputeSatisfiability(BoolValue *Val, + const Environment &Env) { + if (!Val) + return SR::Nullptr; + + if (isa(Val)) + return SR::Top; + + if (Val == &Env.getBoolLiteralValue(true)) + return SR::True; + + if (Val == &Env.getBoolLiteralValue(false)) + return SR::False; + + return SR::Unknown; +} + +// Computes satisfiability by using the flow condition. Slower, but more +// precise. +SatisfiabilityResult computeSatisfiability(BoolValue *Val, + const Environment &Env) { + // Early return with the fast compute values. + if (SatisfiabilityResult ShallowResult = + shallowComputeSatisfiability(Val, Env); + ShallowResult != SR::Unknown) { + return ShallowResult; + } + + if (Env.proves(Val->formula())) + return SR::True; + + if (Env.proves(Env.arena().makeNot(Val->formula()))) + return SR::False; + + return SR::Unknown; +} + +inline BoolValue &getVal(llvm::StringRef Name, Value &PtrValue) { + return *cast(PtrValue.getProperty(Name)); +} + +// Assigns initial pointer null- and nonnull-values to a given Value. +void initializeNullnessProperties(Value &PtrValue, Environment &Env) { + Arena &A = Env.arena(); + + auto *IsNull = cast_or_null(PtrValue.getProperty(kIsNull)); + auto *IsNonnull = cast_or_null(PtrValue.getProperty(kIsNonnull)); + + if (!IsNull) { + IsNull = &A.makeAtomValue(); + PtrValue.setProperty(kIsNull, *IsNull); + } + + if (!IsNonnull) { + IsNonnull = &A.makeAtomValue(); + PtrValue.setProperty(kIsNonnull, *IsNonnull); + } + + // If the pointer cannot have either a null or nonnull value, the state is + // unreachable. + // FIXME: This condition is added in all cases when getValue() is called. + // The reason is that on a post-visit step, the initialized Values are used, + // but the flow condition does not have this constraint yet. + // The framework provides deduplication for constraints, so should not have a + // performance impact. + Env.assume(A.makeOr(IsNull->formula(), IsNonnull->formula())); +} + +void setGLValue(const Expr &E, Value &Val, Environment &Env) { + StorageLocation *SL = Env.getStorageLocation(E); + if (!SL) { + SL = &Env.createStorageLocation(E); + Env.setStorageLocation(E, *SL); + } + + Env.setValue(*SL, Val); +} + +void setUnknownValue(const Expr &E, Value &Val, Environment &Env) { + if (E.isGLValue()) + setGLValue(E, Val, Env); + else + Env.setValue(E, Val); +} + +// Gets a pointer's value, and initializes it to (Unknown, Unknown) if it hasn't +// been initialized already. +Value *getValue(const Expr &Var, Environment &Env) { + if (Value *EnvVal = Env.getValue(Var)) { + // FIXME: The framework usually creates the values for us, but without the + // null-properties. + initializeNullnessProperties(*EnvVal, Env); + + return EnvVal; + } + + return nullptr; +} + +bool hasTopOrNullValue(const Value *Val, const Environment &Env) { + return !Val || isa_and_present(Val->getProperty(kIsNull)) || + isa_and_present(Val->getProperty(kIsNonnull)); +} + +void matchDereferenceExpr(const Stmt *stmt, + const MatchFinder::MatchResult &Result, + Environment &Env) { + const auto *Var = Result.Nodes.getNodeAs(kVar); + assert(Var != nullptr); + + Value *PtrValue = getValue(*Var, Env); + if (hasTopOrNullValue(PtrValue, Env)) + return; + + BoolValue &IsNull = getVal(kIsNull, *PtrValue); + + Env.assume(Env.arena().makeNot(IsNull.formula())); +} + +void matchNullCheckExpr(const Expr *NullCheck, + const MatchFinder::MatchResult &Result, + Environment &Env) { + const auto *Var = Result.Nodes.getNodeAs(kVar); + assert(Var != nullptr); + + // (bool)p or (p != nullptr) + bool IsNonnullOp = true; + if (const auto *BinOp = dyn_cast(NullCheck); + BinOp->getOpcode() == BO_EQ) { + IsNonnullOp = false; + } + + Value *PtrValue = getValue(*Var, Env); + if (hasTopOrNullValue(PtrValue, Env)) + return; + + Arena &A = Env.arena(); + BoolValue &IsNonnull = getVal(kIsNonnull, *PtrValue); + BoolValue &IsNull = getVal(kIsNull, *PtrValue); + + BoolValue *CondValue = cast_or_null(Env.getValue(*NullCheck)); + if (!CondValue) { + CondValue = &A.makeAtomValue(); + Env.setValue(*NullCheck, *CondValue); + } + const Formula &CondFormula = IsNonnullOp ? CondValue->formula() + : A.makeNot(CondValue->formula()); + + Env.assume(A.makeImplies(CondFormula, A.makeNot(IsNull.formula()))); + Env.assume(A.makeImplies(A.makeNot(CondFormula), + A.makeNot(IsNonnull.formula()))); +} + +void matchEqualExpr(const BinaryOperator *EqualExpr, + const MatchFinder::MatchResult &Result, + Environment &Env) { + bool IsNotEqualsOp = EqualExpr->getOpcode() == BO_NE; + + const auto *LHSVar = Result.Nodes.getNodeAs(kVar); + assert(LHSVar != nullptr); + + const auto *RHSVar = Result.Nodes.getNodeAs(kValue); + assert(RHSVar != nullptr); + + Arena &A = Env.arena(); + Value *LHSValue = getValue(*LHSVar, Env); + Value *RHSValue = getValue(*RHSVar, Env); + + if (hasTopOrNullValue(LHSValue, Env) || hasTopOrNullValue(RHSValue, Env)) + return; + + BoolValue &LHSNonnull = getVal(kIsNonnull, *LHSValue); + BoolValue &LHSNull = getVal(kIsNull, *LHSValue); + BoolValue &RHSNonnull = getVal(kIsNonnull, *RHSValue); + BoolValue &RHSNull = getVal(kIsNull, *RHSValue); + + BoolValue *CondValue = cast_or_null(Env.getValue(*EqualExpr)); + if (!CondValue) { + CondValue = &A.makeAtomValue(); + Env.setValue(*EqualExpr, *CondValue); + } + + const Formula &CondFormula = IsNotEqualsOp ? A.makeNot(CondValue->formula()) + : CondValue->formula(); + + // FIXME: Simplify formulas + // If the pointers are equal, the nullability properties are the same. + Env.assume(A.makeImplies(CondFormula, + A.makeAnd(A.makeEquals(LHSNull.formula(), RHSNull.formula()), + A.makeEquals(LHSNonnull.formula(), RHSNonnull.formula())))); + + // If the pointers are not equal, at most one of the pointers is null. + Env.assume(A.makeImplies(A.makeNot(CondFormula), + A.makeNot(A.makeAnd(LHSNull.formula(), RHSNull.formula())))); +} + +void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result, + Environment &Env) { + const auto *PrVar = Result.Nodes.getNodeAs(kVar); + assert(PrVar != nullptr); + + Value *PtrValue = Env.getValue(*PrVar); + if (!PtrValue) { + PtrValue = Env.createValue(PrVar->getType()); + assert(PtrValue && "Failed to create nullptr value"); + Env.setValue(*PrVar, *PtrValue); + } + + PtrValue->setProperty(kIsNull, Env.getBoolLiteralValue(true)); + PtrValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false)); +} + +void matchAddressofExpr(const Expr *expr, + const MatchFinder::MatchResult &Result, + Environment &Env) { + const auto *Var = Result.Nodes.getNodeAs(kVar); + assert(Var != nullptr); + + // FIXME: Use atoms or export to separate function + Value *PtrValue = Env.getValue(*Var); + if (!PtrValue) { + PtrValue = Env.createValue(Var->getType()); + + if (!PtrValue) + return; + + setUnknownValue(*Var, *PtrValue, Env); + } + + PtrValue->setProperty(kIsNull, Env.getBoolLiteralValue(false)); + PtrValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true)); +} + +void matchPtrArgFunctionExpr(const CallExpr *fncall, + const MatchFinder::MatchResult &Result, + Environment &Env) { + for (const auto *Arg : fncall->arguments()) { + // FIXME: Add handling for reference types as arguments + if (Arg->getType()->isPointerType()) { + PointerValue *OuterValue = cast_or_null( + Env.getValue(*Arg)); + + if (!OuterValue) + continue; + + QualType InnerType = Arg->getType()->getPointeeType(); + if (!InnerType->isPointerType()) + continue; + + StorageLocation &InnerLoc = OuterValue->getPointeeLoc(); + + PointerValue *InnerValue = + cast_or_null(Env.getValue(InnerLoc)); + + if (!InnerValue) + continue; + + Value *NewValue = Env.createValue(InnerType); + assert(NewValue && "Failed to re-initialize a pointer's value"); + + Env.setValue(InnerLoc, *NewValue); + + // FIXME: Recursively invalidate all member pointers of eg. a struct + // Should be part of the framework, most likely. + } + } + + if (fncall->getCallReturnType(*Result.Context)->isPointerType() && + !Env.getValue(*fncall)) { + Value *PtrValue = Env.createValue( + fncall->getCallReturnType(*Result.Context)); + if (!PtrValue) + return; + + setUnknownValue(*fncall, *PtrValue, Env); + } +} + +void matchAnyPointerExpr(const Expr *fncall, + const MatchFinder::MatchResult &Result, + Environment &Env) { + // This is not necessarily a prvalue, since operators such as prefix ++ are + // lvalues. + const auto *Var = Result.Nodes.getNodeAs(kVar); + assert(Var != nullptr); + + // In some cases, prvalues are not automatically initialized + // Initialize these values, but don't set null-ness values for performance + if (Env.getValue(*Var)) + return; + + Value *PtrValue = Env.createValue(Var->getType()); + if (!PtrValue) + return; + + setUnknownValue(*Var, *PtrValue, Env); +} + +Diagnoser::ResultType +diagnoseDerefLocation(const Expr *Deref, const MatchFinder::MatchResult &Result, + Diagnoser::DiagnoseArgs &Data) { + auto [ValToDerefLoc, WarningLocToVal, Env] = Data; + + const auto *Var = Result.Nodes.getNodeAs(kVar); + assert(Var != nullptr); + + Value *PtrValue = Env.getValue(*Var); + if (!PtrValue) + return {}; + + // Dereferences are always the highest priority when giving a single location + // FIXME: Do not replace other dereferences, only other Expr's + auto It = ValToDerefLoc.try_emplace(PtrValue, nullptr).first; + + It->second = Deref; + + return {}; +} + +Diagnoser::ResultType diagnoseAssignLocation(const Expr *Assign, + const MatchFinder::MatchResult &Result, + Diagnoser::DiagnoseArgs &Data) { + auto [ValToDerefLoc, WarningLocToVal, Env] = Data; + + const auto *RHSVar = Result.Nodes.getNodeAs(kValue); + assert(RHSVar != nullptr); + + Value *RHSValue = Env.getValue(*RHSVar); + if (!RHSValue) + return {}; + + auto [It, Inserted] = ValToDerefLoc.try_emplace(RHSValue, nullptr); + + if (Inserted) + It->second = Assign; + + return {}; +} + +NullCheckAfterDereferenceDiagnoser::ResultType +diagnoseNullCheckExpr(const Expr *NullCheck, + const MatchFinder::MatchResult &Result, + const Diagnoser::DiagnoseArgs &Data) { + auto [ValToDerefLoc, WarningLocToVal, Env] = Data; + + const auto *Var = Result.Nodes.getNodeAs(kVar); + assert(Var != nullptr); + + if (Value *PtrValue = Env.getValue(*Var)) { + // FIXME: The framework usually creates the values for us, but without the + // nullability properties. + if (PtrValue->getProperty(kIsNull) && PtrValue->getProperty(kIsNonnull)) { + bool IsNull = Env.allows(getVal(kIsNull, *PtrValue).formula()); + bool IsNonnull = Env.allows(getVal(kIsNonnull, *PtrValue).formula()); + + if (!IsNull && IsNonnull) { + // FIXME: Separate function + bool Inserted = + WarningLocToVal.try_emplace(Var->getBeginLoc(), PtrValue).second; + assert(Inserted && "multiple warnings at the same source location"); + (void)Inserted; + + return {{Var->getBeginLoc(), Diagnoser::DiagnosticType::CheckAfterDeref}}; + } + + if (IsNull && !IsNonnull) { + bool Inserted = + WarningLocToVal.try_emplace(Var->getBeginLoc(), PtrValue).second; + assert(Inserted && "multiple warnings at the same source location"); + (void)Inserted; + + return {{Var->getBeginLoc(), Diagnoser::DiagnosticType::CheckWhenNull}}; + } + } + + // If no matches are found, the null-check itself signals a special location + auto [It, Inserted] = ValToDerefLoc.try_emplace(PtrValue, nullptr); + + if (Inserted) + It->second = NullCheck; + } + + return {}; +} + +NullCheckAfterDereferenceDiagnoser::ResultType +diagnoseEqualExpr(const Expr *PtrCheck, const MatchFinder::MatchResult &Result, + Diagnoser::DiagnoseArgs &Data) { + auto [ValToDerefLoc, WarningLocToVal, Env] = Data; + + const auto *LHSVar = Result.Nodes.getNodeAs(kVar); + assert(LHSVar != nullptr); + const auto *RHSVar = Result.Nodes.getNodeAs(kValue); + assert(RHSVar != nullptr); + + Arena &A = Env.arena(); + llvm::SmallVector NullVarLocations; + + if (Value *LHSValue = Env.getValue(*LHSVar); + LHSValue->getProperty(kIsNonnull) && + Env.proves(A.makeNot(getVal(kIsNonnull, *LHSValue).formula()))) { + WarningLocToVal.try_emplace(LHSVar->getBeginLoc(), LHSValue); + NullVarLocations.push_back({LHSVar->getBeginLoc(), Diagnoser::DiagnosticType::CheckWhenNull}); + } + + if (Value *RHSValue = Env.getValue(*RHSVar); + RHSValue->getProperty(kIsNonnull) && + Env.proves(A.makeNot(getVal(kIsNonnull, *RHSValue).formula()))) { + WarningLocToVal.try_emplace(RHSVar->getBeginLoc(), RHSValue); + NullVarLocations.push_back({RHSVar->getBeginLoc(), Diagnoser::DiagnosticType::CheckWhenNull}); + } + + return NullVarLocations; +} + +auto buildTransferMatchSwitch() { + return CFGMatchSwitchBuilder() + .CaseOfCFGStmt(derefMatcher(), matchDereferenceExpr) + .CaseOfCFGStmt(arrowMatcher(), matchDereferenceExpr) + .CaseOfCFGStmt(nullptrMatcher(), matchNullptrExpr) + .CaseOfCFGStmt(addressofMatcher(), matchAddressofExpr) + .CaseOfCFGStmt(functionCallMatcher(), matchPtrArgFunctionExpr) + .CaseOfCFGStmt(anyPointerMatcher(), matchAnyPointerExpr) + .CaseOfCFGStmt(castExprMatcher(), matchNullCheckExpr) + .CaseOfCFGStmt(nullCheckExprMatcher(), matchNullCheckExpr) + .CaseOfCFGStmt(equalExprMatcher(), matchEqualExpr) + .Build(); +} + +auto buildDiagnoseMatchSwitch() { + return CFGMatchSwitchBuilder() + .CaseOfCFGStmt(derefMatcher(), diagnoseDerefLocation) + .CaseOfCFGStmt(arrowMatcher(), diagnoseDerefLocation) + .CaseOfCFGStmt(assignMatcher(), diagnoseAssignLocation) + .CaseOfCFGStmt(castExprMatcher(), diagnoseNullCheckExpr) + .CaseOfCFGStmt(nullCheckExprMatcher(), diagnoseNullCheckExpr) + .CaseOfCFGStmt(equalExprMatcher(), diagnoseEqualExpr) + .Build(); +} + +} // namespace + +NullPointerAnalysisModel::NullPointerAnalysisModel(ASTContext &Context) + : DataflowAnalysis(Context), + TransferMatchSwitch(buildTransferMatchSwitch()) {} + +ast_matchers::StatementMatcher NullPointerAnalysisModel::ptrValueMatcher() { + return ptrWithBinding(); +} + +void NullPointerAnalysisModel::transfer(const CFGElement &E, NoopLattice &State, + Environment &Env) { + TransferMatchSwitch(E, getASTContext(), Env); +} + +void NullPointerAnalysisModel::join(QualType Type, const Value &Val1, + const Environment &Env1, const Value &Val2, + const Environment &Env2, Value &MergedVal, + Environment &MergedEnv) { + if (!Type->isAnyPointerType()) + return; + + const auto MergeValues = [&](llvm::StringRef Name) -> BoolValue & { + auto *LHSVar = cast_or_null(Val1.getProperty(Name)); + auto *RHSVar = cast_or_null(Val2.getProperty(Name)); + + const auto SimplifyVar = [&](BoolValue *VarToSimplify, + const Environment &Env) -> BoolValue * { + SatisfiabilityResult SatResult = + computeSatisfiability(VarToSimplify, Env); + switch (SatResult) { + case SR::Nullptr: + return nullptr; + case SR::Top: + return &MergedEnv.makeTopBoolValue(); + case SR::True: + return &MergedEnv.getBoolLiteralValue(true); + case SR::False: + return &MergedEnv.getBoolLiteralValue(false); + case SR::Unknown: + return VarToSimplify; + } + }; + + LHSVar = SimplifyVar(LHSVar, Env1); + LHSVar = SimplifyVar(RHSVar, Env2); + + // Handle special cases. + if (LHSVar == RHSVar) + return LHSVar ? *LHSVar : MergedEnv.makeAtomicBoolValue(); + else if (isa(LHSVar) || isa(RHSVar)) + return MergedEnv.makeTopBoolValue(); + + if (!LHSVar) + LHSVar = &MergedEnv.makeAtomicBoolValue(); + + if (!RHSVar) + RHSVar = &MergedEnv.makeAtomicBoolValue(); + + assert(LHSVar != nullptr && RHSVar != nullptr); + + if (MergedEnv.proves( + MergedEnv.arena().makeEquals(LHSVar->formula(), RHSVar->formula()))) + return *LHSVar; + + BoolValue &ReturnVar = MergedEnv.makeAtomicBoolValue(); + Arena &A = MergedEnv.arena(); + + MergedEnv.assume(A.makeOr( + A.makeAnd(A.makeAtomRef(Env1.getFlowConditionToken()), + A.makeEquals(ReturnVar.formula(), LHSVar->formula())), + A.makeAnd(A.makeAtomRef(Env2.getFlowConditionToken()), + A.makeEquals(ReturnVar.formula(), RHSVar->formula())))); + + return ReturnVar; + }; + + BoolValue &NonnullValue = MergeValues(kIsNonnull); + BoolValue &NullValue = MergeValues(kIsNull); + + if (isa(NonnullValue) || isa(NullValue)) { + MergedVal.setProperty(kIsNonnull, MergedEnv.makeTopBoolValue()); + MergedVal.setProperty(kIsNull, MergedEnv.makeTopBoolValue()); + } else { + MergedVal.setProperty(kIsNonnull, NonnullValue); + MergedVal.setProperty(kIsNull, NullValue); + + MergedEnv.assume(MergedEnv.makeOr(NonnullValue, NullValue).formula()); + } +} + +ComparisonResult NullPointerAnalysisModel::compare(QualType Type, + const Value &Val1, + const Environment &Env1, + const Value &Val2, + const Environment &Env2) { + + if (!Type->isAnyPointerType()) + return ComparisonResult::Unknown; + + // Evaluate values, but different values compare to Unknown. + auto CompareValues = [&](llvm::StringRef Name) -> CR { + auto *LHSVar = cast_or_null(Val1.getProperty(Name)); + auto *RHSVar = cast_or_null(Val2.getProperty(Name)); + + if (isa_and_present(LHSVar) || + isa_and_present(RHSVar)) + return CR::Top; + + if (LHSVar == RHSVar) + return CR::Same; + + SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1); + SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2); + + if (LHSResult == SR::Top || RHSResult == SR::Top) + return CR::Top; + + if (LHSResult == SR::Unknown || RHSResult == SR::Unknown) + return CR::Unknown; + + if (LHSResult == RHSResult) + return CR::Same; + + return CR::Different; + }; + + CR NullComparison = CompareValues(kIsNull); + CR NonnullComparison = CompareValues(kIsNonnull); + + if (NullComparison == CR::Top || NonnullComparison == CR::Top) + return ComparisonResult::Same; + + if (NullComparison == CR::Different || + NonnullComparison == CR::Different) + return ComparisonResult::Different; + + if (NullComparison == CR::Unknown || + NonnullComparison == CR::Unknown) + return ComparisonResult::Unknown; + + return ComparisonResult::Same; +} + +// Different in that it replaces differing boolean values with Top. +ComparisonResult compareAndReplace(QualType Type, Value &Val1, + const Environment &Env1, Value &Val2, + Environment &Env2) { + + if (!Type->isAnyPointerType()) + return ComparisonResult::Unknown; + + auto FastCompareValues = [&](llvm::StringRef Name) -> CR { + auto *LHSVar = cast_or_null(Val1.getProperty(Name)); + auto *RHSVar = cast_or_null(Val2.getProperty(Name)); + + SatisfiabilityResult LHSResult = shallowComputeSatisfiability(LHSVar, Env1); + SatisfiabilityResult RHSResult = shallowComputeSatisfiability(RHSVar, Env2); + + if (LHSResult == SR::Top || RHSResult == SR::Top) { + Val2.setProperty(Name, Env2.makeTopBoolValue()); + return CR::Top; + } + + if (LHSResult == SR::Unknown || RHSResult == SR::Unknown) + return CR::Unknown; + + if (LHSResult == RHSResult) + return CR::Same; + + Val2.setProperty(Name, Env2.makeTopBoolValue()); + return CR::Different; + }; + + CR NullComparison = FastCompareValues(kIsNull); + CR NonnullComparison = FastCompareValues(kIsNonnull); + + if (NullComparison == CR::Top || NonnullComparison == CR::Top) + return ComparisonResult::Same; + + if (NullComparison == CR::Different || + NonnullComparison == CR::Different) + return ComparisonResult::Different; + + if (NullComparison == CR::Unknown || + NonnullComparison == CR::Unknown) + return ComparisonResult::Unknown; + + return ComparisonResult::Same; +} + +std::optional +NullPointerAnalysisModel::widen(QualType Type, Value &Prev, + const Environment &PrevEnv, Value &Current, + Environment &CurrentEnv) { + if (!Type->isAnyPointerType()) + return std::nullopt; + + switch (compareAndReplace(Type, Prev, PrevEnv, Current, CurrentEnv)) { + case ComparisonResult::Same: + return WidenResult{&Prev, LatticeEffect::Unchanged}; + case ComparisonResult::Unknown: + return std::nullopt; + case ComparisonResult::Different: + return WidenResult{&Current, LatticeEffect::Changed}; + } +} + +NullCheckAfterDereferenceDiagnoser::NullCheckAfterDereferenceDiagnoser() + : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch()) {} + +NullCheckAfterDereferenceDiagnoser::ResultType +NullCheckAfterDereferenceDiagnoser::operator()( + const CFGElement &Elt, ASTContext &Ctx, + const TransferStateForDiagnostics &State) { + DiagnoseArgs Args = {ValToDerefLoc, WarningLocToVal, State.Env}; + return DiagnoseMatchSwitch(Elt, Ctx, Args); +} + +} // namespace clang::dataflow diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt index 6c01ae8fc2e54..9c7fdefb1b3d8 100644 --- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt +++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt @@ -17,6 +17,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests MapLatticeTest.cpp MatchSwitchTest.cpp MultiVarConstantPropagationTest.cpp + NullPointerAnalysisModelTest.cpp RecordOpsTest.cpp SignAnalysisTest.cpp SimplifyConstraintsTest.cpp diff --git a/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp new file mode 100644 index 0000000000000..7480c5b5f4292 --- /dev/null +++ b/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp @@ -0,0 +1,290 @@ +//===- NullPointerAnalysisModelTest.cpp -------------------------*- 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 +// +//===----------------------------------------------------------------------===// +// +// This file defines a test for pointer nullability, specifically focused on +// finding invalid dereferences, and unnecessary null-checks. +// Only a limited set of operations are currently recognized. Notably, pointer +// arithmetic, null-pointer assignments and _nullable/_nonnull attributes are +// missing as of yet. +// +// FIXME: Port over to the new type of dataflow test infrastructure +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h" +#include "TestingSupport.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/Stmt.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h" +#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" +#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" +#include "clang/Analysis/FlowSensitive/DataflowLattice.h" +#include "clang/Analysis/FlowSensitive/MapLattice.h" +#include "clang/Analysis/FlowSensitive/NoopLattice.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/Twine.h" +#include "llvm/Support/Error.h" +#include "llvm/Testing/ADT/StringMapEntry.h" +#include "llvm/Testing/Support/Error.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include +#include +#include +#include +#include + +namespace clang::dataflow { +namespace { +using namespace ast_matchers; + +constexpr char kVar[] = "var"; +// constexpr char kKnown[] = "is-known"; +constexpr char kIsNonnull[] = "is-nonnull"; +constexpr char kIsNull[] = "is-null"; + +constexpr char kBoolTrue[] = "true"; +constexpr char kBoolFalse[] = "false"; +constexpr char kBoolInvalid[] = "invalid"; +constexpr char kBoolUnknown[] = "unknown"; +constexpr char kBoolNullptr[] = "is-nullptr"; + +std::string checkNullabilityState(BoolValue *value, const Environment &Env) { + if (value == nullptr) { + return std::string(kBoolNullptr); + } else { + int boolState = 0; + if (Env.proves(value->formula())) { + boolState |= 1; + } + if (Env.proves(Env.makeNot(*value).formula())) { + boolState |= 2; + } + switch (boolState) { + case 0: + return kBoolUnknown; + case 1: + return kBoolTrue; + case 2: + return kBoolFalse; + // If both the condition and its negation are satisfied, the program point + // is proven to be impossible. + case 3: + return kBoolInvalid; + default: + llvm_unreachable("all cases covered in switch"); + } + } +} + +using namespace test; +using ::llvm::IsStringMapEntry; +using ::testing::AllOf; +using ::testing::Args; +using ::testing::IsSupersetOf; +using ::testing::NotNull; +using ::testing::Pair; +using ::testing::UnorderedElementsAre; + +MATCHER_P2(HasNullabilityState, null, nonnull, + std::string("has nullability state where isNull is ") + null + + " and isNonnull is " + nonnull) { + return checkNullabilityState( + cast_or_null(arg.first->getProperty(kIsNonnull)), + *arg.second) == nonnull && + checkNullabilityState( + cast_or_null(arg.first->getProperty(kIsNull)), + *arg.second) == null; +} + +MATCHER_P3(HoldsVariable, name, output, checks, + ((negation ? "doesn't hold" : "holds") + + llvm::StringRef(" a variable in its environment that ") + + ::testing::DescribeMatcher>( + checks, negation)) + .str()) { + auto MatchResults = + match(functionDecl(hasDescendant(namedDecl(hasName(name)).bind(kVar))), + *output.Target, output.ASTCtx); + assert(!MatchResults.empty()); + + const auto *pointerExpr = MatchResults[0].template getNodeAs(kVar); + assert(pointerExpr != nullptr); + + const auto *ExprValue = arg.Env.getValue(*pointerExpr); + + if (ExprValue == nullptr) { + return false; + } + + return ExplainMatchResult(checks, std::pair{ExprValue, &arg.Env}, + result_listener); +} + +void RunDataflowAnalysis( + llvm::StringRef Code, + std::function> &, + const AnalysisOutputs &)> + VerifyResults) { + ASSERT_THAT_ERROR(checkDataflow( + AnalysisInputs( + Code, hasName("fun"), + [](ASTContext &C, Environment &Env) { + return NullPointerAnalysisModel(C); + }) + .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}), + VerifyResults), + llvm::Succeeded()); +} + +template +void ExpectDataflowResult(llvm::StringRef Code, MatcherFactory Expectations) { + RunDataflowAnalysis( + Code, + /*VerifyResults=*/ + [&Expectations](const llvm::StringMap> &Results, + const AnalysisOutputs &Output) { + EXPECT_THAT(Results, Expectations(Output)); + }); +} + +TEST(NullCheckAfterDereferenceTest, Operations) { + std::string Code = R"( + struct S { + int a; + }; + + void fun(int *Deref, S *Arrow) { + *Deref = 0; + Arrow->a = 20; + // [[p]] + } + )"; + ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto { + return UnorderedElementsAre(IsStringMapEntry( + "p", AllOf(HoldsVariable("Deref", Output, + HasNullabilityState(kBoolFalse, kBoolTrue)), + HoldsVariable("Arrow", Output, + HasNullabilityState(kBoolFalse, kBoolTrue))))); + }); +} + +TEST(NullCheckAfterDereferenceTest, Conditional) { + std::string Code = R"( + void fun(int *p) { + if (p) { + (void)0; // [[p_true]] + } else { + (void)0; // [[p_false]] + } + + (void)0; // [[p_merge]] + } + )"; + ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto { + return UnorderedElementsAre( + IsStringMapEntry("p_true", HoldsVariable("p", Output, + HasNullabilityState( + kBoolFalse, kBoolTrue))), + IsStringMapEntry("p_false", HoldsVariable("p", Output, + HasNullabilityState( + kBoolTrue, kBoolFalse))), + IsStringMapEntry( + "p_merge", + HoldsVariable("p", Output, + HasNullabilityState(kBoolUnknown, kBoolUnknown)))); + }); +} + +TEST(NullCheckAfterDereferenceTest, ValueAssignment) { + std::string Code = R"( + using size_t = decltype(sizeof(void*)); + extern void *malloc(size_t); + extern int *ext(); + + void fun(int arg) { + int *Addressof = &arg; + int *Nullptr = nullptr; + int *Nullptr2 = 0; + + int *MallocExpr = (int *)malloc(sizeof(int)); + int *NewExpr = new int(3); + + int *ExternalFn = ext(); + + (void)0; // [[p]] + } + )"; + ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto { + return UnorderedElementsAre(IsStringMapEntry( + "p", + AllOf(HoldsVariable("Addressof", Output, + HasNullabilityState(kBoolFalse, kBoolTrue)), + HoldsVariable("Nullptr", Output, + HasNullabilityState(kBoolTrue, kBoolFalse)), + HoldsVariable("Nullptr", Output, + HasNullabilityState(kBoolTrue, kBoolFalse)), + HoldsVariable("MallocExpr", Output, + HasNullabilityState(kBoolNullptr, kBoolNullptr)), + // HoldsVariable("NewExpr", Output, + // HasNullabilityState(kBoolFalse, kBoolTrue)), + HoldsVariable("ExternalFn", Output, + HasNullabilityState(kBoolNullptr, kBoolNullptr))))); + }); +} + +TEST(NullCheckAfterDereferenceTest, BooleanDependence) { + std::string Code = R"( + void fun(int *ptr, bool b) { + if (b) { + *ptr = 10; + } else { + ptr = nullptr; + } + + (void)0; // [[p]] + } + )"; + RunDataflowAnalysis(Code, [](const llvm::StringMap> &Results, + const AnalysisOutputs &Outputs) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + auto &BoolVal = getValueForDecl(Outputs.ASTCtx, Env, "b"); + auto &PtrVal = getValueForDecl(Outputs.ASTCtx, Env, "ptr"); + + auto *IsNull = cast_or_null(PtrVal.getProperty(kIsNull)); + auto *IsNonnull = cast_or_null(PtrVal.getProperty(kIsNonnull)); + ASSERT_THAT(IsNull, NotNull()); + ASSERT_THAT(IsNonnull, NotNull()); + + ASSERT_EQ(checkNullabilityState( + &Env.makeImplication(BoolVal, Env.makeNot(*IsNull)), Env), + kBoolTrue); + ASSERT_EQ(checkNullabilityState( + &Env.makeImplication(Env.makeNot(BoolVal), *IsNull), Env), + kBoolTrue); + ASSERT_EQ( + checkNullabilityState(&Env.makeImplication(BoolVal, *IsNonnull), Env), + kBoolTrue); + ASSERT_EQ( + checkNullabilityState(&Env.makeImplication(*IsNonnull, BoolVal), Env), + kBoolTrue); + }); +} + +} // namespace +} // namespace clang::dataflow