Skip to content

Commit 5ca8775

Browse files
balazskebalazske
authored
[clang][analyzer] Fix argument invalidations in StreamChecker. (#79470)
Specific arguments passed to stream handling functions are changed by the function, this means these should be invalidated ("escaped") by the analyzer. This change adds the argument invalidation (in specific cases) to the checker.
1 parent 6fae3e7 commit 5ca8775

File tree

2 files changed

+185
-1
lines changed

2 files changed

+185
-1
lines changed

‎clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
2222
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
2323
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
24+
#include "llvm/ADT/Sequence.h"
2425
#include <functional>
2526
#include <optional>
2627

@@ -629,6 +630,21 @@ const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
629630
return nullptr;
630631
}
631632

633+
static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
634+
const CallEvent &Call,
635+
ArrayRef<unsigned int> EscapingArgs) {
636+
const auto *CE = Call.getOriginExpr();
637+
638+
SmallVector<SVal> EscapingVals;
639+
EscapingVals.reserve(EscapingArgs.size());
640+
for (auto EscArgIdx : EscapingArgs)
641+
EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
642+
State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
643+
C.getLocationContext(),
644+
/*CausesPointerEscape=*/false);
645+
return State;
646+
}
647+
632648
//===----------------------------------------------------------------------===//
633649
// Methods of StreamChecker.
634650
//===----------------------------------------------------------------------===//
@@ -819,6 +835,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
819835
return;
820836
}
821837

838+
// At read, invalidate the buffer in any case of error or success,
839+
// except if EOF was already present.
840+
if (IsFread && !E.isStreamEof())
841+
State = escapeArgs(State, C, Call, {0});
842+
822843
// Generate a transition for the success state.
823844
// If we know the state to be FEOF at fread, do not add a success state.
824845
if (!IsFread || !E.isStreamEof()) {
@@ -863,6 +884,9 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
863884
return;
864885

865886
if (!E.isStreamEof()) {
887+
// If there was already EOF, assume that read buffer is not changed.
888+
// Otherwise it may change at success or failure.
889+
State = escapeArgs(State, C, Call, {0});
866890
if (SingleChar) {
867891
// Generate a transition for the success state of `fgetc`.
868892
NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
@@ -1011,6 +1035,14 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
10111035
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
10121036
StateNotFailed =
10131037
E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
1038+
if (!StateNotFailed)
1039+
return;
1040+
1041+
SmallVector<unsigned int> EscArgs;
1042+
for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
1043+
EscArgs.push_back(EscArg);
1044+
StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
1045+
10141046
if (StateNotFailed)
10151047
C.addTransition(StateNotFailed);
10161048
}
@@ -1073,8 +1105,12 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
10731105
// return -1.
10741106
// If an error occurs, the function shall return -1 and set 'errno'.
10751107

1076-
// Add transition for the successful state.
10771108
if (!E.isStreamEof()) {
1109+
// Escape buffer and size (may change by the call).
1110+
// May happen even at error (partial read?).
1111+
State = escapeArgs(State, C, Call, {0, 1});
1112+
1113+
// Add transition for the successful state.
10781114
NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
10791115
ProgramStateRef StateNotFailed =
10801116
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
@@ -1161,6 +1197,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
11611197

11621198
ProgramStateRef StateNotFailed, StateFailed;
11631199
std::tie(StateFailed, StateNotFailed) = E.makeRetValAndAssumeDual(State, C);
1200+
StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
11641201

11651202
// This function does not affect the stream state.
11661203
// Still we add success and failure state with the appropriate return value.

‎clang/test/Analysis/stream-invalidate.c

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
// RUN: %clang_analyze_cc1 -verify %s \
2+
// RUN: -analyzer-checker=core \
3+
// RUN: -analyzer-checker=alpha.unix.Stream \
4+
// RUN: -analyzer-checker=debug.ExprInspection
5+
6+
#include "Inputs/system-header-simulator.h"
7+
8+
void clang_analyzer_eval(int);
9+
void clang_analyzer_dump(int);
10+
11+
void test_fread(void) {
12+
FILE *F = fopen("file", "r+");
13+
if (!F)
14+
return;
15+
16+
char Buf[3] = {10, 10, 10};
17+
fread(Buf, 1, 3, F);
18+
// The check applies to success and failure.
19+
clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
20+
clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
21+
if (feof(F)) {
22+
char Buf1[3] = {10, 10, 10};
23+
fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
24+
clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
25+
clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
26+
}
27+
28+
fclose(F);
29+
}
30+
31+
void test_fwrite(void) {
32+
FILE *F = fopen("file", "r+");
33+
if (!F)
34+
return;
35+
36+
char Buf[3] = {10, 10, 10};
37+
fwrite(Buf, 1, 3, F);
38+
// The check applies to success and failure.
39+
clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
40+
clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}
41+
42+
fclose(F);
43+
}
44+
45+
void test_fgets() {
46+
FILE *F = tmpfile();
47+
if (!F)
48+
return;
49+
50+
char Buf[3] = {10, 10, 10};
51+
fgets(Buf, 3, F);
52+
// The check applies to success and failure.
53+
clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
54+
clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
55+
if (feof(F)) {
56+
char Buf1[3] = {10, 10, 10};
57+
fgets(Buf1, 3, F); // expected-warning {{is in EOF state}}
58+
clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
59+
clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
60+
}
61+
62+
fclose(F);
63+
}
64+
65+
void test_fputs() {
66+
FILE *F = tmpfile();
67+
if (!F)
68+
return;
69+
70+
char *Buf = "aaa";
71+
fputs(Buf, F);
72+
// The check applies to success and failure.
73+
clang_analyzer_dump(Buf[0]); // expected-warning {{97 S32b}}
74+
clang_analyzer_dump(Buf[2]); // expected-warning {{97 S32b}}
75+
clang_analyzer_dump(Buf[3]); // expected-warning {{0 S32b}}
76+
77+
fclose(F);
78+
}
79+
80+
void test_fscanf() {
81+
FILE *F = tmpfile();
82+
if (!F)
83+
return;
84+
85+
int a = 1;
86+
unsigned b;
87+
int Ret = fscanf(F, "%d %u", &a, &b);
88+
if (Ret == 0) {
89+
clang_analyzer_dump(a); // expected-warning {{conj_$}}
90+
// FIXME: should be {{1 S32b}}.
91+
clang_analyzer_dump(b); // expected-warning {{conj_$}}
92+
// FIXME: should be {{uninitialized value}}.
93+
} else if (Ret == 1) {
94+
clang_analyzer_dump(a); // expected-warning {{conj_$}}
95+
clang_analyzer_dump(b); // expected-warning {{conj_$}}
96+
// FIXME: should be {{uninitialized value}}.
97+
} else if (Ret >= 2) {
98+
clang_analyzer_dump(a); // expected-warning {{conj_$}}
99+
clang_analyzer_dump(b); // expected-warning {{conj_$}}
100+
clang_analyzer_eval(Ret == 2); // expected-warning {{FALSE}} expected-warning {{TRUE}}
101+
// FIXME: should be only TRUE.
102+
} else {
103+
clang_analyzer_dump(a); // expected-warning {{1 S32b}}
104+
clang_analyzer_dump(b); // expected-warning {{uninitialized value}}
105+
}
106+
107+
fclose(F);
108+
}
109+
110+
void test_getdelim(char *P, size_t Sz) {
111+
FILE *F = tmpfile();
112+
if (!F)
113+
return;
114+
115+
char *P1 = P;
116+
size_t Sz1 = Sz;
117+
ssize_t Ret = getdelim(&P, &Sz, '\t', F);
118+
if (Ret < 0) {
119+
clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
120+
// expected-warning {{TRUE}}
121+
clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
122+
// expected-warning {{TRUE}}
123+
} else {
124+
clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
125+
// expected-warning {{TRUE}}
126+
clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
127+
// expected-warning {{TRUE}}
128+
}
129+
130+
fclose(F);
131+
}
132+
133+
void test_fgetpos() {
134+
FILE *F = tmpfile();
135+
if (!F)
136+
return;
137+
138+
fpos_t Pos = 1;
139+
int Ret = fgetpos(F, &Pos);
140+
if (Ret == 0) {
141+
clang_analyzer_dump(Pos); // expected-warning {{conj_$}}
142+
} else {
143+
clang_analyzer_dump(Pos); // expected-warning {{1 S32b}}
144+
}
145+
146+
fclose(F);
147+
}

0 commit comments

Comments
 (0)