Skip to content

Commit d5cd8c5

Browse files
committed
[Analyzer][WebKit] UncountedLocalVarsChecker
Differential Review: https://reviews.llvm.org/D83259
1 parent 52215bb commit d5cd8c5

File tree

5 files changed

+401
-0
lines changed

5 files changed

+401
-0
lines changed

clang/docs/analyzer/checkers.rst

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2501,6 +2501,53 @@ We also define a set of safe transformations which if passed a safe value as an
25012501
- casts
25022502
- unary operators like ``&`` or ``*``
25032503
2504+
alpha.webkit.UncountedLocalVarsChecker
2505+
""""""""""""""""""""""""""""""""""""""
2506+
The goal of this rule is to make sure that any uncounted local variable is backed by a ref-counted object with lifetime that is strictly larger than the scope of the uncounted local variable. To be on the safe side we require the scope of an uncounted variable to be embedded in the scope of ref-counted object that backs it.
2507+
2508+
These are examples of cases that we consider safe:
2509+
2510+
.. code-block:: cpp
2511+
void foo1() {
2512+
RefPtr<RefCountable> counted;
2513+
// The scope of uncounted is EMBEDDED in the scope of counted.
2514+
{
2515+
RefCountable* uncounted = counted.get(); // ok
2516+
}
2517+
}
2518+
2519+
void foo2(RefPtr<RefCountable> counted_param) {
2520+
RefCountable* uncounted = counted_param.get(); // ok
2521+
}
2522+
2523+
void FooClass::foo_method() {
2524+
RefCountable* uncounted = this; // ok
2525+
}
2526+
2527+
Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that an argument is safe or it's considered if not a bug then bug-prone.
2528+
2529+
.. code-block:: cpp
2530+
2531+
void foo1() {
2532+
RefCountable* uncounted = new RefCountable; // warn
2533+
}
2534+
2535+
RefCountable* global_uncounted;
2536+
void foo2() {
2537+
RefCountable* uncounted = global_uncounted; // warn
2538+
}
2539+
2540+
void foo3() {
2541+
RefPtr<RefCountable> counted;
2542+
// The scope of uncounted is not EMBEDDED in the scope of counted.
2543+
RefCountable* uncounted = counted.get(); // warn
2544+
}
2545+
2546+
We don't warn about these cases - we don't consider them necessarily safe but since they are very common and usually safe we'd introduce a lot of false positives otherwise:
2547+
- variable defined in condition part of an ```if``` statement
2548+
- variable defined in init statement condition of a ```for``` statement
2549+
2550+
For the time being we also don't warn about uninitialized uncounted local variables.
25042551
25052552
Debug Checkers
25062553
---------------

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1666,4 +1666,8 @@ def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
16661666
HelpText<"Check uncounted call arguments.">,
16671667
Documentation<HasDocumentation>;
16681668

1669+
def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
1670+
HelpText<"Check uncounted local variables.">,
1671+
Documentation<HasDocumentation>;
1672+
16691673
} // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ add_clang_library(clangStaticAnalyzerCheckers
128128
WebKit/RefCntblBaseVirtualDtorChecker.cpp
129129
WebKit/UncountedCallArgsChecker.cpp
130130
WebKit/UncountedLambdaCapturesChecker.cpp
131+
WebKit/UncountedLocalVarsChecker.cpp
131132

132133
LINK_LIBS
133134
clangAST
Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
//=======- UncountedLocalVarsChecker.cpp -------------------------*- C++ -*-==//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "ASTUtils.h"
10+
#include "DiagOutputUtils.h"
11+
#include "PtrTypesSemantics.h"
12+
#include "clang/AST/CXXInheritance.h"
13+
#include "clang/AST/Decl.h"
14+
#include "clang/AST/DeclCXX.h"
15+
#include "clang/AST/ParentMapContext.h"
16+
#include "clang/AST/RecursiveASTVisitor.h"
17+
#include "clang/Basic/SourceLocation.h"
18+
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
19+
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
20+
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
21+
#include "clang/StaticAnalyzer/Core/Checker.h"
22+
#include "llvm/ADT/DenseSet.h"
23+
24+
using namespace clang;
25+
using namespace ento;
26+
27+
namespace {
28+
29+
// for ( int a = ...) ... true
30+
// for ( int a : ...) ... true
31+
// if ( int* a = ) ... true
32+
// anything else ... false
33+
bool isDeclaredInForOrIf(const VarDecl *Var) {
34+
assert(Var);
35+
auto &ASTCtx = Var->getASTContext();
36+
auto parent = ASTCtx.getParents(*Var);
37+
38+
if (parent.size() == 1) {
39+
if (auto *DS = parent.begin()->get<DeclStmt>()) {
40+
DynTypedNodeList grandParent = ASTCtx.getParents(*DS);
41+
if (grandParent.size() == 1) {
42+
return grandParent.begin()->get<ForStmt>() ||
43+
grandParent.begin()->get<IfStmt>() ||
44+
grandParent.begin()->get<CXXForRangeStmt>();
45+
}
46+
}
47+
}
48+
return false;
49+
}
50+
51+
// FIXME: should be defined by anotations in the future
52+
bool isRefcountedStringsHack(const VarDecl *V) {
53+
assert(V);
54+
auto safeClass = [](const std::string &className) {
55+
return className == "String" || className == "AtomString" ||
56+
className == "UniquedString" || className == "Identifier";
57+
};
58+
QualType QT = V->getType();
59+
auto *T = QT.getTypePtr();
60+
if (auto *CXXRD = T->getAsCXXRecordDecl()) {
61+
if (safeClass(safeGetName(CXXRD)))
62+
return true;
63+
}
64+
if (T->isPointerType() || T->isReferenceType()) {
65+
if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
66+
if (safeClass(safeGetName(CXXRD)))
67+
return true;
68+
}
69+
}
70+
return false;
71+
}
72+
73+
bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
74+
const VarDecl *MaybeGuardian) {
75+
assert(Guarded);
76+
assert(MaybeGuardian);
77+
78+
if (!MaybeGuardian->isLocalVarDecl())
79+
return false;
80+
81+
const CompoundStmt *guardiansClosestCompStmtAncestor = nullptr;
82+
83+
ASTContext &ctx = MaybeGuardian->getASTContext();
84+
85+
for (DynTypedNodeList guardianAncestors = ctx.getParents(*MaybeGuardian);
86+
!guardianAncestors.empty();
87+
guardianAncestors = ctx.getParents(
88+
*guardianAncestors
89+
.begin()) // FIXME - should we handle all of the parents?
90+
) {
91+
for (auto &guardianAncestor : guardianAncestors) {
92+
if (auto *CStmtParentAncestor = guardianAncestor.get<CompoundStmt>()) {
93+
guardiansClosestCompStmtAncestor = CStmtParentAncestor;
94+
break;
95+
}
96+
}
97+
if (guardiansClosestCompStmtAncestor)
98+
break;
99+
}
100+
101+
if (!guardiansClosestCompStmtAncestor)
102+
return false;
103+
104+
// We need to skip the first CompoundStmt to avoid situation when guardian is
105+
// defined in the same scope as guarded variable.
106+
bool HaveSkippedFirstCompoundStmt = false;
107+
for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
108+
!guardedVarAncestors.empty();
109+
guardedVarAncestors = ctx.getParents(
110+
*guardedVarAncestors
111+
.begin()) // FIXME - should we handle all of the parents?
112+
) {
113+
for (auto &guardedVarAncestor : guardedVarAncestors) {
114+
if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
115+
if (!HaveSkippedFirstCompoundStmt) {
116+
HaveSkippedFirstCompoundStmt = true;
117+
continue;
118+
}
119+
if (CStmtAncestor == guardiansClosestCompStmtAncestor)
120+
return true;
121+
}
122+
}
123+
}
124+
125+
return false;
126+
}
127+
128+
class UncountedLocalVarsChecker
129+
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
130+
BugType Bug{this,
131+
"Uncounted raw pointer or reference not provably backed by "
132+
"ref-counted variable",
133+
"WebKit coding guidelines"};
134+
mutable BugReporter *BR;
135+
136+
public:
137+
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
138+
BugReporter &BRArg) const {
139+
BR = &BRArg;
140+
141+
// The calls to checkAST* from AnalysisConsumer don't
142+
// visit template instantiations or lambda classes. We
143+
// want to visit those, so we make our own RecursiveASTVisitor.
144+
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
145+
const UncountedLocalVarsChecker *Checker;
146+
explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
147+
: Checker(Checker) {
148+
assert(Checker);
149+
}
150+
151+
bool shouldVisitTemplateInstantiations() const { return true; }
152+
bool shouldVisitImplicitCode() const { return false; }
153+
154+
bool VisitVarDecl(VarDecl *V) {
155+
Checker->visitVarDecl(V);
156+
return true;
157+
}
158+
};
159+
160+
LocalVisitor visitor(this);
161+
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
162+
}
163+
164+
void visitVarDecl(const VarDecl *V) const {
165+
if (shouldSkipVarDecl(V))
166+
return;
167+
168+
const auto *ArgType = V->getType().getTypePtr();
169+
if (!ArgType)
170+
return;
171+
172+
if (isUncountedPtr(ArgType)) {
173+
const Expr *const InitExpr = V->getInit();
174+
if (!InitExpr)
175+
return; // FIXME: later on we might warn on uninitialized vars too
176+
177+
const clang::Expr *const InitArgOrigin =
178+
tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false)
179+
.first;
180+
if (!InitArgOrigin)
181+
return;
182+
183+
if (isa<CXXThisExpr>(InitArgOrigin))
184+
return;
185+
186+
if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
187+
if (auto *MaybeGuardian =
188+
dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
189+
const auto *MaybeGuardianArgType =
190+
MaybeGuardian->getType().getTypePtr();
191+
if (!MaybeGuardianArgType)
192+
return;
193+
const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
194+
MaybeGuardianArgType->getAsCXXRecordDecl();
195+
if (!MaybeGuardianArgCXXRecord)
196+
return;
197+
198+
if (MaybeGuardian->isLocalVarDecl() &&
199+
(isRefCounted(MaybeGuardianArgCXXRecord) ||
200+
isRefcountedStringsHack(MaybeGuardian)) &&
201+
isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
202+
return;
203+
}
204+
205+
// Parameters are guaranteed to be safe for the duration of the call
206+
// by another checker.
207+
if (isa<ParmVarDecl>(MaybeGuardian))
208+
return;
209+
}
210+
}
211+
212+
reportBug(V);
213+
}
214+
}
215+
216+
bool shouldSkipVarDecl(const VarDecl *V) const {
217+
assert(V);
218+
if (!V->isLocalVarDecl())
219+
return true;
220+
221+
if (isDeclaredInForOrIf(V))
222+
return true;
223+
224+
return false;
225+
}
226+
227+
void reportBug(const VarDecl *V) const {
228+
assert(V);
229+
SmallString<100> Buf;
230+
llvm::raw_svector_ostream Os(Buf);
231+
232+
Os << "Local variable ";
233+
printQuotedQualifiedName(Os, V);
234+
Os << " is uncounted and unsafe.";
235+
236+
PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
237+
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
238+
Report->addRange(V->getSourceRange());
239+
BR->emitReport(std::move(Report));
240+
}
241+
};
242+
} // namespace
243+
244+
void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
245+
Mgr.registerChecker<UncountedLocalVarsChecker>();
246+
}
247+
248+
bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) {
249+
return true;
250+
}

0 commit comments

Comments
 (0)