Skip to content

Commit 60f3b07

Browse files
committed
[clang][analyzer] Add checker for bad use of 'errno'.
Extend checker 'ErrnoModeling' with a state of 'errno' to indicate the importance of the 'errno' value and how it should be used. Add a new checker 'ErrnoChecker' that observes use of 'errno' and finds possible wrong uses, based on the "errno state". The "errno state" should be set (together with value of 'errno') by other checkers (that perform modeling of the given function) in the future. Currently only a test function can set this value. The new checker has no user-observable effect yet. Reviewed By: martong, steakhal Differential Revision: https://reviews.llvm.org/D122150
1 parent 0ad4f29 commit 60f3b07

File tree

12 files changed

+817
-16
lines changed

12 files changed

+817
-16
lines changed

clang/docs/ReleaseNotes.rst

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,10 @@ Static Analyzer
581581
`strcmp``, ``strncmp``, ``strcpy``, ``strlen``, ``strsep`` and many more. Although
582582
this checker currently is in list of alpha checkers due to a false positive.
583583

584+
- Added a new checker ``alpha.unix.Errno``. This can find the first read
585+
of ``errno`` after successful standard function calls, such use of ``errno``
586+
could be unsafe.
587+
584588
- Deprecate the ``-analyzer-store region`` and
585589
``-analyzer-opt-analyze-nested-blocks`` analyzer flags.
586590
These flags are still accepted, but a warning will be displayed.

clang/docs/analyzer/checkers.rst

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2520,6 +2520,75 @@ Check improper use of chroot.
25202520
f(); // warn: no call of chdir("/") immediately after chroot
25212521
}
25222522
2523+
.. _alpha-unix-Errno:
2524+
2525+
alpha.unix.Errno (C)
2526+
""""""""""""""""""""
2527+
2528+
Check for improper use of ``errno``.
2529+
This checker implements partially CERT rule
2530+
`ERR30-C. Set errno to zero before calling a library function known to set errno,
2531+
and check errno only after the function returns a value indicating failure
2532+
<https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152351>`_.
2533+
The checker can find the first read of ``errno`` after successful standard
2534+
function calls.
2535+
2536+
The C and POSIX standards often do not define if a standard library function
2537+
may change value of ``errno`` if the call does not fail.
2538+
Therefore, ``errno`` should only be used if it is known from the return value
2539+
of a function that the call has failed.
2540+
There are exceptions to this rule (for example ``strtol``) but the affected
2541+
functions are not yet supported by the checker.
2542+
The return values for the failure cases are documented in the standard Linux man
2543+
pages of the functions and in the `POSIX standard <https://pubs.opengroup.org/onlinepubs/9699919799/>`_.
2544+
2545+
.. code-block:: c
2546+
2547+
int unsafe_errno_read(int sock, void *data, int data_size) {
2548+
if (send(sock, data, data_size, 0) != data_size) {
2549+
// 'send' can be successful even if not all data was sent
2550+
if (errno == 1) { // An undefined value may be read from 'errno'
2551+
return 0;
2552+
}
2553+
}
2554+
return 1;
2555+
}
2556+
2557+
The supported functions are the same that are modeled by checker
2558+
:ref:`alpha-unix-StdCLibraryFunctionArgs`.
2559+
The ``ModelPOSIX`` option of that checker affects the set of checked functions.
2560+
2561+
**Parameters**
2562+
2563+
The ``AllowErrnoReadOutsideConditionExpressions`` option allows read of the
2564+
errno value if the value is not used in a condition (in ``if`` statements,
2565+
loops, conditional expressions, ``switch`` statements). For example ``errno``
2566+
can be stored into a variable without getting a warning by the checker.
2567+
2568+
.. code-block:: c
2569+
2570+
int unsafe_errno_read(int sock, void *data, int data_size) {
2571+
if (send(sock, data, data_size, 0) != data_size) {
2572+
int err = errno;
2573+
// warning if 'AllowErrnoReadOutsideConditionExpressions' is false
2574+
// no warning if 'AllowErrnoReadOutsideConditionExpressions' is true
2575+
}
2576+
return 1;
2577+
}
2578+
2579+
Default value of this option is ``true``. This allows save of the errno value
2580+
for possible later error handling.
2581+
2582+
**Limitations**
2583+
2584+
- Only the very first usage of ``errno`` is checked after an affected function
2585+
call. Value of ``errno`` is not followed when it is stored into a variable
2586+
or returned from a function.
2587+
- Documentation of function ``lseek`` is not clear about what happens if the
2588+
function returns different value than the expected file position but not -1.
2589+
To avoid possible false-positives ``errno`` is allowed to be used in this
2590+
case.
2591+
25232592
.. _alpha-unix-PthreadLock:
25242593
25252594
alpha.unix.PthreadLock (C)

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -546,6 +546,18 @@ def VforkChecker : Checker<"Vfork">,
546546

547547
let ParentPackage = UnixAlpha in {
548548

549+
def ErrnoChecker : Checker<"Errno">,
550+
HelpText<"Check for improper use of 'errno'">,
551+
Dependencies<[ErrnoModeling]>,
552+
CheckerOptions<[
553+
CmdLineOption<Boolean,
554+
"AllowErrnoReadOutsideConditionExpressions",
555+
"Allow read of undefined value from errno outside of conditions",
556+
"true",
557+
InAlpha>,
558+
]>,
559+
Documentation<HasDocumentation>;
560+
549561
def ChrootChecker : Checker<"Chroot">,
550562
HelpText<"Check improper use of chroot">,
551563
Documentation<HasDocumentation>;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ add_clang_library(clangStaticAnalyzerCheckers
4040
DynamicTypePropagation.cpp
4141
DynamicTypeChecker.cpp
4242
EnumCastOutOfRangeChecker.cpp
43+
ErrnoChecker.cpp
4344
ErrnoModeling.cpp
4445
ErrnoTesterChecker.cpp
4546
ExprInspectionChecker.cpp
Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,249 @@
1+
//=== ErrnoChecker.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+
// This defines an "errno checker" that can detect some invalid use of the
10+
// system-defined value 'errno'. This checker works together with the
11+
// ErrnoModeling checker and other checkers like StdCLibraryFunctions.
12+
//
13+
//===----------------------------------------------------------------------===//
14+
15+
#include "ErrnoModeling.h"
16+
#include "clang/AST/ParentMapContext.h"
17+
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
18+
#include "clang/StaticAnalyzer/Core/Checker.h"
19+
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
20+
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
21+
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
22+
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
23+
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
24+
#include "llvm/ADT/STLExtras.h"
25+
26+
using namespace clang;
27+
using namespace ento;
28+
using namespace errno_modeling;
29+
30+
namespace {
31+
32+
class ErrnoChecker
33+
: public Checker<check::Location, check::PreCall, check::RegionChanges> {
34+
public:
35+
void checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
36+
CheckerContext &) const;
37+
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
38+
ProgramStateRef
39+
checkRegionChanges(ProgramStateRef State,
40+
const InvalidatedSymbols *Invalidated,
41+
ArrayRef<const MemRegion *> ExplicitRegions,
42+
ArrayRef<const MemRegion *> Regions,
43+
const LocationContext *LCtx, const CallEvent *Call) const;
44+
void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const;
45+
46+
/// Indicates if a read (load) of \c errno is allowed in a non-condition part
47+
/// of \c if, \c switch, loop and conditional statements when the errno
48+
/// value may be undefined.
49+
bool AllowErrnoReadOutsideConditions = true;
50+
51+
private:
52+
void generateErrnoNotCheckedBug(CheckerContext &C, ProgramStateRef State,
53+
const MemRegion *ErrnoRegion,
54+
const CallEvent *CallMayChangeErrno) const;
55+
56+
BugType BT_InvalidErrnoRead{this, "Value of 'errno' could be undefined",
57+
"Error handling"};
58+
BugType BT_ErrnoNotChecked{this, "Value of 'errno' was not checked",
59+
"Error handling"};
60+
};
61+
62+
} // namespace
63+
64+
static ProgramStateRef setErrnoStateIrrelevant(ProgramStateRef State) {
65+
return setErrnoState(State, Irrelevant);
66+
}
67+
68+
/// Check if a statement (expression) or an ancestor of it is in a condition
69+
/// part of a (conditional, loop, switch) statement.
70+
static bool isInCondition(const Stmt *S, CheckerContext &C) {
71+
ParentMapContext &ParentCtx = C.getASTContext().getParentMapContext();
72+
bool CondFound = false;
73+
while (S && !CondFound) {
74+
const DynTypedNodeList Parents = ParentCtx.getParents(*S);
75+
if (Parents.empty())
76+
break;
77+
const auto *ParentS = Parents[0].get<Stmt>();
78+
if (!ParentS || isa<CallExpr>(ParentS))
79+
break;
80+
switch (ParentS->getStmtClass()) {
81+
case Expr::IfStmtClass:
82+
CondFound = (S == cast<IfStmt>(ParentS)->getCond());
83+
break;
84+
case Expr::ForStmtClass:
85+
CondFound = (S == cast<ForStmt>(ParentS)->getCond());
86+
break;
87+
case Expr::DoStmtClass:
88+
CondFound = (S == cast<DoStmt>(ParentS)->getCond());
89+
break;
90+
case Expr::WhileStmtClass:
91+
CondFound = (S == cast<WhileStmt>(ParentS)->getCond());
92+
break;
93+
case Expr::SwitchStmtClass:
94+
CondFound = (S == cast<SwitchStmt>(ParentS)->getCond());
95+
break;
96+
case Expr::ConditionalOperatorClass:
97+
CondFound = (S == cast<ConditionalOperator>(ParentS)->getCond());
98+
break;
99+
case Expr::BinaryConditionalOperatorClass:
100+
CondFound = (S == cast<BinaryConditionalOperator>(ParentS)->getCommon());
101+
break;
102+
default:
103+
break;
104+
}
105+
S = ParentS;
106+
}
107+
return CondFound;
108+
}
109+
110+
void ErrnoChecker::generateErrnoNotCheckedBug(
111+
CheckerContext &C, ProgramStateRef State, const MemRegion *ErrnoRegion,
112+
const CallEvent *CallMayChangeErrno) const {
113+
if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
114+
SmallString<100> StrBuf;
115+
llvm::raw_svector_ostream OS(StrBuf);
116+
if (CallMayChangeErrno) {
117+
OS << "Value of 'errno' was not checked and may be overwritten by "
118+
"function '";
119+
const auto *CallD =
120+
dyn_cast_or_null<FunctionDecl>(CallMayChangeErrno->getDecl());
121+
assert(CallD && CallD->getIdentifier());
122+
OS << CallD->getIdentifier()->getName() << "'";
123+
} else {
124+
OS << "Value of 'errno' was not checked and is overwritten here";
125+
}
126+
auto BR = std::make_unique<PathSensitiveBugReport>(BT_ErrnoNotChecked,
127+
OS.str(), N);
128+
BR->markInteresting(ErrnoRegion);
129+
C.emitReport(std::move(BR));
130+
}
131+
}
132+
133+
void ErrnoChecker::checkLocation(SVal Loc, bool IsLoad, const Stmt *S,
134+
CheckerContext &C) const {
135+
Optional<ento::Loc> ErrnoLoc = getErrnoLoc(C.getState());
136+
if (!ErrnoLoc)
137+
return;
138+
139+
auto L = Loc.getAs<ento::Loc>();
140+
if (!L || *ErrnoLoc != *L)
141+
return;
142+
143+
ProgramStateRef State = C.getState();
144+
ErrnoCheckState EState = getErrnoState(State);
145+
146+
if (IsLoad) {
147+
switch (EState) {
148+
case MustNotBeChecked:
149+
// Read of 'errno' when it may have undefined value.
150+
if (!AllowErrnoReadOutsideConditions || isInCondition(S, C)) {
151+
if (ExplodedNode *N = C.generateErrorNode()) {
152+
auto BR = std::make_unique<PathSensitiveBugReport>(
153+
BT_InvalidErrnoRead,
154+
"An undefined value may be read from 'errno'", N);
155+
BR->markInteresting(ErrnoLoc->getAsRegion());
156+
C.emitReport(std::move(BR));
157+
}
158+
}
159+
break;
160+
case MustBeChecked:
161+
// 'errno' has to be checked. A load is required for this, with no more
162+
// information we can assume that it is checked somehow.
163+
// After this place 'errno' is allowed to be read and written.
164+
State = setErrnoStateIrrelevant(State);
165+
C.addTransition(State);
166+
break;
167+
default:
168+
break;
169+
}
170+
} else {
171+
switch (EState) {
172+
case MustBeChecked:
173+
// 'errno' is overwritten without a read before but it should have been
174+
// checked.
175+
generateErrnoNotCheckedBug(C, setErrnoStateIrrelevant(State),
176+
ErrnoLoc->getAsRegion(), nullptr);
177+
break;
178+
case MustNotBeChecked:
179+
// Write to 'errno' when it is not allowed to be read.
180+
// After this place 'errno' is allowed to be read and written.
181+
State = setErrnoStateIrrelevant(State);
182+
C.addTransition(State);
183+
break;
184+
default:
185+
break;
186+
}
187+
}
188+
}
189+
190+
void ErrnoChecker::checkPreCall(const CallEvent &Call,
191+
CheckerContext &C) const {
192+
const auto *CallF = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
193+
if (!CallF)
194+
return;
195+
196+
CallF = CallF->getCanonicalDecl();
197+
// If 'errno' must be checked, it should be done as soon as possible, and
198+
// before any other call to a system function (something in a system header).
199+
// To avoid use of a long list of functions that may change 'errno'
200+
// (which may be different with standard library versions) assume that any
201+
// function can change it.
202+
// A list of special functions can be used that are allowed here without
203+
// generation of diagnostic. For now the only such case is 'errno' itself.
204+
// Probably 'strerror'?
205+
if (CallF->isExternC() && CallF->isGlobal() &&
206+
C.getSourceManager().isInSystemHeader(CallF->getLocation()) &&
207+
!isErrno(CallF)) {
208+
if (getErrnoState(C.getState()) == MustBeChecked) {
209+
Optional<ento::Loc> ErrnoLoc = getErrnoLoc(C.getState());
210+
assert(ErrnoLoc && "ErrnoLoc should exist if an errno state is set.");
211+
generateErrnoNotCheckedBug(C, setErrnoStateIrrelevant(C.getState()),
212+
ErrnoLoc->getAsRegion(), &Call);
213+
}
214+
}
215+
}
216+
217+
ProgramStateRef ErrnoChecker::checkRegionChanges(
218+
ProgramStateRef State, const InvalidatedSymbols *Invalidated,
219+
ArrayRef<const MemRegion *> ExplicitRegions,
220+
ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx,
221+
const CallEvent *Call) const {
222+
Optional<ento::Loc> ErrnoLoc = getErrnoLoc(State);
223+
if (!ErrnoLoc)
224+
return State;
225+
const MemRegion *ErrnoRegion = ErrnoLoc->getAsRegion();
226+
227+
// If 'errno' is invalidated we can not know if it is checked or written into,
228+
// allow read and write without bug reports.
229+
if (llvm::is_contained(Regions, ErrnoRegion))
230+
return setErrnoStateIrrelevant(State);
231+
232+
// Always reset errno state when the system memory space is invalidated.
233+
// The ErrnoRegion is not always found in the list in this case.
234+
if (llvm::is_contained(Regions, ErrnoRegion->getMemorySpace()))
235+
return setErrnoStateIrrelevant(State);
236+
237+
return State;
238+
}
239+
240+
void ento::registerErrnoChecker(CheckerManager &mgr) {
241+
const AnalyzerOptions &Opts = mgr.getAnalyzerOptions();
242+
auto *Checker = mgr.registerChecker<ErrnoChecker>();
243+
Checker->AllowErrnoReadOutsideConditions = Opts.getCheckerBooleanOption(
244+
Checker, "AllowErrnoReadOutsideConditionExpressions");
245+
}
246+
247+
bool ento::shouldRegisterErrnoChecker(const CheckerManager &mgr) {
248+
return true;
249+
}

0 commit comments

Comments
 (0)