Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1699,6 +1699,21 @@ int d = &y - &x; // warn } +alpha.core.ReverseNull (C) +""""""""""""""""""""""""" +Checks whether a dereferenced (and as such, assumed to be non-null) pointer is +present in a condition. + +.. code-block:: c + + int *get(); + void top() { + int *p = get(); + int x = *p; // note: pointer assumed non-null here + if (p) // warn: pointer already constrained nonnull + return; + } + .. _alpha-core-SizeofPtr: alpha.core.SizeofPtr (C) Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -298,6 +298,11 @@ Dependencies<[PthreadLockBase]>, Documentation; +def ReverseNullChecker : Checker<"ReverseNull">, + HelpText<"Checks whether a pointer that is already known to null or non-null " + "is present in a condition.">, + Documentation; + } // end "alpha.core" //===----------------------------------------------------------------------===// @@ -1431,6 +1436,12 @@ "false", Released, Hide>, + CmdLineOption, CmdLineOption, check::PreCall, check::PostCall, check::EndFunction, check::EndAnalysis, check::NewAllocator, check::Bind, check::PointerEscape, check::RegionChanges, - check::LiveSymbols, eval::Call> { + check::LiveSymbols, eval::Call, eval::Assume> { bool isCallbackEnabled(const AnalyzerOptions &Opts, StringRef CallbackName) const { @@ -213,6 +213,13 @@ llvm::errs() << "PointerEscape\n"; return State; } + + ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond, + bool Assumption) const { + if (isCallbackEnabled(State, "EvalAssume")) + llvm::errs() << "EvalAssume\n"; + return State; + } }; } // end anonymous namespace Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -76,6 +76,7 @@ NonNullParamChecker.cpp NonnullGlobalConstantsChecker.cpp NullabilityChecker.cpp + NullPtrInterferenceChecker.cpp NumberObjectConversionChecker.cpp ObjCAtSyncChecker.cpp ObjCAutoreleaseWriteChecker.cpp Index: clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp @@ -0,0 +1,244 @@ +//===--- ReverseNullChecker.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 +// +//===----------------------------------------------------------------------===// +// +// TODO +// +//===----------------------------------------------------------------------===// + +#include "clang/AST/Stmt.h" +#include "clang/AST/StmtObjC.h" +#include "clang/Analysis/AnalysisDeclContext.h" +#include "clang/Analysis/PathDiagnostic.h" +#include "clang/Analysis/ProgramPoint.h" +#include "clang/Basic/AttrKinds.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h" +#include "llvm/Support/ErrorHandling.h" +#include + +using namespace clang; +using namespace ento; + +REGISTER_MAP_WITH_PROGRAMSTATE(NonNullConstrainedPtrs, const SymbolRef, + const LocationContext *) + +static bool isConstrainedNonNull(ProgramStateRef State, const MemRegion *MR) { + return State->isNonNull(loc::MemRegionVal(MR)).isConstrainedTrue(); +} + +static bool isNodeBeforeNonNullConstraint(const ExplodedNode *N, + const MemRegion *MR) { + assert(N); + assert(N->getFirstPred()); + return !isConstrainedNonNull(N->getFirstPred()->getState(), MR) && + isConstrainedNonNull(N->getState(), MR); +} + +/// Leave a note about where the constraint to null/non-null was imposed. +class ReverseNullVisitor : public BugReporterVisitor { + /// The memory region that was a part of a condition where its value was + /// already known. + const MemRegion *MR; + bool IsSatisfied = false; + +public: + ReverseNullVisitor(const MemRegion *MR) : MR(MR) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int x = 0; + ID.AddPointer(&x); + } + + static void *getTag() { + static int Tag = 0; + return static_cast(&Tag); + } + + PathDiagnosticPieceRef VisitNode(const ExplodedNode *N, + BugReporterContext &BC, + PathSensitiveBugReport &R) override { + if (IsSatisfied) + return nullptr; + + if (!isNodeBeforeNonNullConstraint(N, MR)) + return nullptr; + + IsSatisfied = true; + + return std::make_shared( + PathDiagnosticLocation{N->getNextStmtForDiagnostics(), + BC.getSourceManager(), N->getLocationContext()}, + "Pointer assumed non-null here"); + } + + /// The checker must have made sure that this node actually exists. If we did + /// not find it, we must've messed up the visitor. Are we sure that we used + /// the same machinery to find the exploded here AND in the checker? + virtual void finalizeVisitor(BugReporterContext &BRC, + const ExplodedNode *EndPathNode, + PathSensitiveBugReport &BR) override { + assert(IsSatisfied && + "Failed to find the ExplodedNode where the constaint was imposed on " + "the condition whose value was known!"); + } +}; + +/// Climb up the ExplodedGraph, and find the ExplodedNode where \p MR was +/// constrained to be non-null. Ideally, this would be done with a +/// BugReporterVisitor, but the ExplodedGraph it has access to is NOT the same +/// as the ExplodedGraph that is constructed during analysis +/// (see https://reviews.llvm.org/D65379). In fact, it is a linear graph, and +/// we're specifically interested in branches, hence the unusual approach. +static const ExplodedNode *getNBeforeNonNullConstraint(const ExplodedNode *N, + const MemRegion *MR) { + + assert(!N->getFirstSucc() && + "This should be a leaf of the ExplodedGraph (at this point in the " + "analysis)!"); + + assert(isConstrainedNonNull(N->getState(), MR) && + "This pointer should be non-null!"); + + N = N->getFirstPred(); + + // Look for the node where the constraint was imposed. + while (N->getFirstPred() && !isNodeBeforeNonNullConstraint(N, MR)) + N = N->getFirstPred(); + + // We failed to find the node. This can happen with ObjC self, which may not + // have a node where its non-null, even if it can be non-null. + if (!N) + return nullptr; + + const ExplodedNode *BeforeNonNullConstraintN = N->getFirstPred(); + if (!BeforeNonNullConstraintN) + return nullptr; + + assert(!isConstrainedNonNull(BeforeNonNullConstraintN->getState(), MR) && + "This is supposed to be the node where the constraint is not yet " + "imposed!"); + + return BeforeNonNullConstraintN; +} + +/// Check whether the point where the condition was contrained to be +/// null/non-null was a state split to analyze (at least) two new paths of +/// execution, or only one. For example, here, p is null on one path, and +/// non-null on another where the constraints are imposed: +/// +/// int *p = get(); +/// if (p) // state split, this ExplodedNode will have two children +/// // p assumed non-null +/// else +/// // p assumed non-null +/// //... +/// if (p) // Don't warn here, this is a totally valid condition +/// // ... +/// +/// But here, on all paths immediately after p's dereference, p is non-null: +/// +/// int *p = get(); +/// *p = 5; // This ExplodedNode will have a single child +/// if (p) // Warn here! +/// // ... +/// +/// Mind that the node unconditionally leads down a single path, if all but one +/// child is a sink node. +static bool unconditionallyLeadsHere(const ExplodedNode *N) { + size_t NonSinkNodeCount = llvm::count_if( + N->succs(), [](const ExplodedNode *N) { return !N->isSink(); }); + + assert(NonSinkNodeCount != 0 && + "At least one node should be non-sinking, because one leads to the " + "path of execution the checker currently analyses!"); + + // TODO: Actually assert whether this child leads to the node where the currnt + // analysis is. + return NonSinkNodeCount == 1; +} + +namespace { + +class ReverseNullChecker : public Checker { + BugType BT; + +public: + ReverseNullChecker() + : BT(this, "Pointer is unconditionally non-null here", "Reverse null") {} + + void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const { + if (isa(Condition)) + return; + + auto Cond = Ctx.getSVal(Condition).getAs(); + if (!Cond) + return; + + const MemRegion *MR = Cond->getAsRegion(); + if (!MR) + return; + + // TODO: Altough not as interesting, we could also check the null case. + if (isConstrainedNonNull(Ctx.getState(), MR)) { + + const ExplodedNode *NBeforeConstraint = + getNBeforeNonNullConstraint(Ctx.getPredecessor(), MR); + if (!NBeforeConstraint) + return; + + if (!unconditionallyLeadsHere(NBeforeConstraint)) + return; + + // We want to be sure that the constraint and the condition are in the + // same stackframe. Caller and callee functions' pre/post conditions may + // not apply to the caller stackframe. A similar issue is discussed here: + // https://discourse.llvm.org/t/static-analyzer-query-why-is-suppress-null-return-paths-enabled-by-default/ + if (NBeforeConstraint->getLocationContext() != Ctx.getLocationContext()) + return; + + const ExplodedNode *N = Ctx.generateNonFatalErrorNode(Ctx.getState()); + if (!N) + return; + + std::unique_ptr R( + std::make_unique(BT, BT.getDescription(), N)); + R->addNote( + "Consider moving the condition here", + PathDiagnosticLocation{NBeforeConstraint->getNextStmtForDiagnostics(), + Ctx.getSourceManager(), + NBeforeConstraint->getLocationContext()}); + R->addVisitor(MR); + bugreporter::trackExpressionValue(N, cast(Condition), *R); + Ctx.emitReport(std::move(R)); + } + } +}; + +} // namespace + +void clang::ento::registerReverseNullChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool clang::ento::shouldRegisterReverseNullChecker(const CheckerManager &mgr) { + return true; +} Index: clang/test/Analysis/NSString.m =================================================================== --- clang/test/Analysis/NSString.m +++ clang/test/Analysis/NSString.m @@ -1,7 +1,31 @@ -// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -verify -Wno-objc-root-class %s -// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -analyzer-config mode=shallow -verify -Wno-objc-root-class %s -// RUN: %clang_analyze_cc1 -DTEST_64 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -verify -Wno-objc-root-class %s -// RUN: %clang_analyze_cc1 -DOSATOMIC_USE_INLINED -triple i386-apple-darwin10 -analyzer-checker=core,osx.cocoa.NilArg,osx.cocoa.RetainCount,alpha.core -analyzer-store=region -verify -Wno-objc-root-class %s +// RUN: %clang_analyze_cc1 -verify %s\ +// RUN: -triple i386-apple-darwin10 -Wno-objc-root-class \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=osx.cocoa.NilArg \ +// RUN: -analyzer-checker=osx.cocoa.RetainCount \ +// RUN: -analyzer-checker=alpha.core + +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -triple i386-apple-darwin10 -Wno-objc-root-class \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=osx.cocoa.NilArg \ +// RUN: -analyzer-checker=osx.cocoa.RetainCount \ +// RUN: -analyzer-checker=alpha.core \ +// RUN: -analyzer-config mode=shallow + +// RUN: %clang_analyze_cc1 -verify %s -DTEST_64 \ +// RUN: -triple x86_64-apple-darwin10 -Wno-objc-root-class \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=osx.cocoa.NilArg \ +// RUN: -analyzer-checker=osx.cocoa.RetainCount \ +// RUN: -analyzer-checker=alpha.core + +// RUN: %clang_analyze_cc1 -verify %s -DOSATOMIC_USE_INLINED \ +// RUN: -triple i386-apple-darwin10 -Wno-objc-root-class \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=osx.cocoa.NilArg \ +// RUN: -analyzer-checker=osx.cocoa.RetainCount \ +// RUN: -analyzer-checker=alpha.core //===----------------------------------------------------------------------===// // The following code is reduced using delta-debugging from @@ -162,8 +186,9 @@ // This exercises the 'EvalAssume' logic in GRTransferFuncs (CFRefCount.cpp). NSString* f11(CFDictionaryRef dict, const char* key) { NSString* s = (NSString*) CFDictionaryGetValue(dict, key); - [s retain]; + [s retain]; // expected-note{{Consider moving the condition here}} if (s) { + // expected-warning@-1{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}} [s release]; } return 0; Index: clang/test/Analysis/analyzer-config.c =================================================================== --- clang/test/Analysis/analyzer-config.c +++ clang/test/Analysis/analyzer-config.c @@ -55,6 +55,7 @@ // CHECK-NEXT: debug.AnalysisOrder:Bind = false // CHECK-NEXT: debug.AnalysisOrder:EndAnalysis = false // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false +// CHECK-NEXT: debug.AnalysisOrder:EvalAssume = false // CHECK-NEXT: debug.AnalysisOrder:EvalCall = false // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false Index: clang/test/Analysis/null-pointer-interference.c =================================================================== --- /dev/null +++ clang/test/Analysis/null-pointer-interference.c @@ -0,0 +1,92 @@ +// RUN: %clang_analyze_cc1 -verify %s -analyzer-output=text\ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-checker=alpha.core.ReverseNull \ +// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \ +// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \ +// RUN: 2>&1 | FileCheck %s + +void clang_analyzer_warnIfReached(); +//===----------------------------------------------------------------------===// +// True positive test cases. +//===----------------------------------------------------------------------===// + +void tp1(int *p) { + *p = 5; + // expected-note@-1{{Pointer assumed non-null here}} + // expected-note@-2{{Consider moving the condition here}} + if (p) + // expected-note@-1{{Pointer is unconditionally non-null here}} + // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}} + return; +} + +//===----------------------------------------------------------------------===// + +void tp2(int *p) { + *p = 5; + // expected-note@-1{{Pointer assumed non-null here}} + // expected-note@-2{{Consider moving the condition here}} + int *x = p; + // expected-note@-1{{'x' initialized here}} + if (x) + // expected-note@-1{{Pointer is unconditionally non-null here}} + // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}} + return; +} + +//===----------------------------------------------------------------------===// + +char *getenv(const char *name); +long a64l(const char *str64); +// CHECK: Loaded summary for: char *getenv(const char *name) +// CHECK: Loaded summary for: long a64l(const char *str64) + +void tp3_posix_nonnull_constraint(const char *p) { + a64l(p); + // expected-note@-1{{Pointer assumed non-null here}} + // expected-note@-2{{Consider moving the condition here}} + if (p) + // expected-note@-1{{Pointer is unconditionally non-null here}} + // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}} + return; +} + +//===----------------------------------------------------------------------===// + +void tp4_nonnull_constraint(const char *p) { + getenv(p); + // expected-note@-1{{Pointer assumed non-null here}} + // expected-note@-2{{Consider moving the condition here}} + if (p) + // expected-note@-1{{Pointer is unconditionally non-null here}} + // expected-warning@-2{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}} + return; +} + +//===----------------------------------------------------------------------===// +// True negative test cases. +//===----------------------------------------------------------------------===// + +void tn1_constrain_arg(int *p) { + *p = 5; +} + +void tn1_constraint_in_nested_stackframe(int *p) { + tn1_constrain_arg(p); + if (p) + return; +} + +//===----------------------------------------------------------------------===// + +void tn2_constraint_in_parent_stackframe(int *p) { + if (p) + return; +} + +void tn2_caller(int *p) { + *p = 5; + tn2_constraint_in_parent_stackframe(p); +} Index: clang/test/Analysis/null-pointer-interference.m =================================================================== --- /dev/null +++ clang/test/Analysis/null-pointer-interference.m @@ -0,0 +1,27 @@ +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -triple i386-apple-darwin10 -Wno-objc-root-class -fblocks \ +// RUN: -analyzer-output=text \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=osx.cocoa.NilArg \ +// RUN: -analyzer-checker=osx.cocoa.RetainCount \ +// RUN: -analyzer-checker=alpha.core.ReverseNull + +#include "Inputs/system-header-simulator-objc.h" + +typedef const struct __CFDictionary * CFDictionaryRef; +const void *CFDictionaryGetValue(CFDictionaryRef theDict, const void *key); + +// From NSString.m: +NSString* f11(CFDictionaryRef dict, const char* key) { + NSString* s = (NSString*) CFDictionaryGetValue(dict, key); + // expected-note@-1{{'s' initialized here}} + [s retain]; + // expected-note@-1{{Pointer assumed non-null here}} + // expected-note@-2{{Consider moving the condition here}} + if (s) { + // expected-warning@-1{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}} + // expected-note@-2{{Pointer is unconditionally non-null here}} + [s release]; + } + return 0; +}