Skip to content

Commit

Permalink
[clang][analyzer] Fix argument invalidations in StreamChecker. (#79470)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
balazske committed Feb 23, 2024
1 parent 6fae3e7 commit 5ca8775
Show file tree
Hide file tree
Showing 2 changed files with 185 additions and 1 deletion.
39 changes: 38 additions & 1 deletion clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
#include "llvm/ADT/Sequence.h"
#include <functional>
#include <optional>

Expand Down Expand Up @@ -629,6 +630,21 @@ const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
return nullptr;
}

static ProgramStateRef escapeArgs(ProgramStateRef State, CheckerContext &C,
const CallEvent &Call,
ArrayRef<unsigned int> EscapingArgs) {
const auto *CE = Call.getOriginExpr();

SmallVector<SVal> EscapingVals;
EscapingVals.reserve(EscapingArgs.size());
for (auto EscArgIdx : EscapingArgs)
EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
C.getLocationContext(),
/*CausesPointerEscape=*/false);
return State;
}

//===----------------------------------------------------------------------===//
// Methods of StreamChecker.
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -819,6 +835,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
return;
}

// At read, invalidate the buffer in any case of error or success,
// except if EOF was already present.
if (IsFread && !E.isStreamEof())
State = escapeArgs(State, C, Call, {0});

// Generate a transition for the success state.
// If we know the state to be FEOF at fread, do not add a success state.
if (!IsFread || !E.isStreamEof()) {
Expand Down Expand Up @@ -863,6 +884,9 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
return;

if (!E.isStreamEof()) {
// If there was already EOF, assume that read buffer is not changed.
// Otherwise it may change at success or failure.
State = escapeArgs(State, C, Call, {0});
if (SingleChar) {
// Generate a transition for the success state of `fgetc`.
NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
Expand Down Expand Up @@ -1011,6 +1035,14 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
StateNotFailed =
E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call));
if (!StateNotFailed)
return;

SmallVector<unsigned int> EscArgs;
for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
EscArgs.push_back(EscArg);
StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);

if (StateNotFailed)
C.addTransition(StateNotFailed);
}
Expand Down Expand Up @@ -1073,8 +1105,12 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
// return -1.
// If an error occurs, the function shall return -1 and set 'errno'.

// Add transition for the successful state.
if (!E.isStreamEof()) {
// Escape buffer and size (may change by the call).
// May happen even at error (partial read?).
State = escapeArgs(State, C, Call, {0, 1});

// Add transition for the successful state.
NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
ProgramStateRef StateNotFailed =
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
Expand Down Expand Up @@ -1161,6 +1197,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,

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

// This function does not affect the stream state.
// Still we add success and failure state with the appropriate return value.
Expand Down
147 changes: 147 additions & 0 deletions clang/test/Analysis/stream-invalidate.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
// RUN: %clang_analyze_cc1 -verify %s \
// RUN: -analyzer-checker=core \
// RUN: -analyzer-checker=alpha.unix.Stream \
// RUN: -analyzer-checker=debug.ExprInspection

#include "Inputs/system-header-simulator.h"

void clang_analyzer_eval(int);
void clang_analyzer_dump(int);

void test_fread(void) {
FILE *F = fopen("file", "r+");
if (!F)
return;

char Buf[3] = {10, 10, 10};
fread(Buf, 1, 3, F);
// The check applies to success and failure.
clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
if (feof(F)) {
char Buf1[3] = {10, 10, 10};
fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
}

fclose(F);
}

void test_fwrite(void) {
FILE *F = fopen("file", "r+");
if (!F)
return;

char Buf[3] = {10, 10, 10};
fwrite(Buf, 1, 3, F);
// The check applies to success and failure.
clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}

fclose(F);
}

void test_fgets() {
FILE *F = tmpfile();
if (!F)
return;

char Buf[3] = {10, 10, 10};
fgets(Buf, 3, F);
// The check applies to success and failure.
clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
if (feof(F)) {
char Buf1[3] = {10, 10, 10};
fgets(Buf1, 3, F); // expected-warning {{is in EOF state}}
clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
}

fclose(F);
}

void test_fputs() {
FILE *F = tmpfile();
if (!F)
return;

char *Buf = "aaa";
fputs(Buf, F);
// The check applies to success and failure.
clang_analyzer_dump(Buf[0]); // expected-warning {{97 S32b}}
clang_analyzer_dump(Buf[2]); // expected-warning {{97 S32b}}
clang_analyzer_dump(Buf[3]); // expected-warning {{0 S32b}}

fclose(F);
}

void test_fscanf() {
FILE *F = tmpfile();
if (!F)
return;

int a = 1;
unsigned b;
int Ret = fscanf(F, "%d %u", &a, &b);
if (Ret == 0) {
clang_analyzer_dump(a); // expected-warning {{conj_$}}
// FIXME: should be {{1 S32b}}.
clang_analyzer_dump(b); // expected-warning {{conj_$}}
// FIXME: should be {{uninitialized value}}.
} else if (Ret == 1) {
clang_analyzer_dump(a); // expected-warning {{conj_$}}
clang_analyzer_dump(b); // expected-warning {{conj_$}}
// FIXME: should be {{uninitialized value}}.
} else if (Ret >= 2) {
clang_analyzer_dump(a); // expected-warning {{conj_$}}
clang_analyzer_dump(b); // expected-warning {{conj_$}}
clang_analyzer_eval(Ret == 2); // expected-warning {{FALSE}} expected-warning {{TRUE}}
// FIXME: should be only TRUE.
} else {
clang_analyzer_dump(a); // expected-warning {{1 S32b}}
clang_analyzer_dump(b); // expected-warning {{uninitialized value}}
}

fclose(F);
}

void test_getdelim(char *P, size_t Sz) {
FILE *F = tmpfile();
if (!F)
return;

char *P1 = P;
size_t Sz1 = Sz;
ssize_t Ret = getdelim(&P, &Sz, '\t', F);
if (Ret < 0) {
clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
// expected-warning {{TRUE}}
clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
// expected-warning {{TRUE}}
} else {
clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
// expected-warning {{TRUE}}
clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
// expected-warning {{TRUE}}
}

fclose(F);
}

void test_fgetpos() {
FILE *F = tmpfile();
if (!F)
return;

fpos_t Pos = 1;
int Ret = fgetpos(F, &Pos);
if (Ret == 0) {
clang_analyzer_dump(Pos); // expected-warning {{conj_$}}
} else {
clang_analyzer_dump(Pos); // expected-warning {{1 S32b}}
}

fclose(F);
}

0 comments on commit 5ca8775

Please sign in to comment.