Skip to content

[clang][analyzer] Add function 'fprintf' to StreamChecker. #77613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 12, 2024
Merged

[clang][analyzer] Add function 'fprintf' to StreamChecker. #77613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
{{{"fputs"}, 2},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, false), 1}},
{{{"fprintf"}},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
{{{"ungetc"}, 2},
{std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
std::bind(&StreamChecker::evalUngetc, _1, _2, _3, _4), 1}},
Expand Down Expand Up @@ -339,6 +342,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
void evalFputx(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C, bool IsSingleChar) const;

void evalFprintf(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;

void evalUngetc(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const;

Expand Down Expand Up @@ -926,6 +932,49 @@ void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
C.addTransition(StateFailed);
}

void StreamChecker::evalFprintf(const FnDescription *Desc,
const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
if (Call.getNumArgs() < 2)
return;
SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
if (!StreamSym)
return;

const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
if (!CE)
return;

const StreamState *OldSS = State->get<StreamMap>(StreamSym);
if (!OldSS)
return;

assertStreamStateOpened(OldSS);

NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
SValBuilder &SVB = C.getSValBuilder();
auto &ACtx = C.getASTContext();
auto Cond = SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ACtx.IntTy),
SVB.getConditionType())
.getAs<DefinedOrUnknownSVal>();
if (!Cond)
return;
ProgramStateRef StateNotFailed, StateFailed;
std::tie(StateNotFailed, StateFailed) = State->assume(*Cond);
Comment on lines +955 to +965
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downstream we used State->assumeInclusiveRange() for this.

As a sideffect, we bound an upperbound to the RetVal with the number of arguments we had for the call.
Have you considered setting the upperbound here too?

I was thinking if the std library functions checker applies this constraint, but AFAIK it does not, nor we have a proposed patch for doing so. And anyways I'd prefer setting both upper and lowerbounds at the same checker if possible. Thus, I write to this PR.

Copy link
Contributor

@balazs-benics-sonarsource balazs-benics-sonarsource Jan 24, 2024 •
edited by steakhal
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this, we should also invalidate any buffers passed to such calls. This also sounds like a regression compared to default eval calling "fpintf". (Look at the format string specifier %n)

EDIT: It's still me (steakhal). I always forget to switch accounts :s

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, some fixes can be done with fprintf and fscanf and the argument invalidation. I plan to do this with the following patches.


StateNotFailed =
StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
C.addTransition(StateNotFailed);

// Add transition for the failed state. The resulting value of the file
// position indicator for the stream is indeterminate.
StateFailed = StateFailed->set<StreamMap>(
StreamSym, StreamState::getOpened(Desc, ErrorFError, true));
C.addTransition(StateFailed);
}

void StreamChecker::evalUngetc(const FnDescription *Desc, const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
Expand Down
17 changes: 17 additions & 0 deletions clang/test/Analysis/stream-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,23 @@ void error_fputs(void) {
fputs("ABC", F); // expected-warning {{Stream might be already closed}}
}

void error_fprintf(void) {
FILE *F = tmpfile();
if (!F)
return;
int Ret = fprintf(F, "aaa");
if (Ret >= 0) {
clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
fprintf(F, "bbb"); // no-warning
} else {
clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
fprintf(F, "bbb"); // expected-warning {{might be 'indeterminate'}}
}
fclose(F);
fprintf(F, "ccc"); // expected-warning {{Stream might be already closed}}
}

void error_ungetc() {
FILE *F = tmpfile();
if (!F)
Expand Down
11 changes: 8 additions & 3 deletions clang/test/Analysis/stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ void check_fputs(void) {
fclose(fp);
}

void check_fprintf(void) {
FILE *fp = tmpfile();
fprintf(fp, "ABC"); // expected-warning {{Stream pointer might be NULL}}
fclose(fp);
}

void check_ungetc(void) {
FILE *fp = tmpfile();
ungetc('A', fp); // expected-warning {{Stream pointer might be NULL}}
Expand Down Expand Up @@ -271,9 +277,8 @@ void check_escape4(void) {
return;
fwrite("1", 1, 1, F); // may fail

// no escape at (non-StreamChecker-handled) system call
// FIXME: all such calls should be handled by the checker
fprintf(F, "0");
// no escape at a non-StreamChecker-handled system call
setbuf(F, "0");

fwrite("1", 1, 1, F); // expected-warning {{might be 'indeterminate'}}
fclose(F);
Expand Down