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

[balazske]

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

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Kéri (balazske)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/77613.diff

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+49)
• (modified) clang/test/Analysis/stream-error.c (+17)
• (modified) clang/test/Analysis/stream.c (+8-3)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 25da3c18e8519f..d6df5199ece2d7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -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}},
       {{{"fseek"}, 3},
        {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{{"ftell"}, 1},
@@ -332,6 +335,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 preFseek(const FnDescription *Desc, const CallEvent &Call,
                 CheckerContext &C) const;
   void evalFseek(const FnDescription *Desc, const CallEvent &Call,
@@ -916,6 +922,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);
+
+  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::preFseek(const FnDescription *Desc, const CallEvent &Call,
                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 13c6684b5840af..abe391b21a4d07 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -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 write_after_eof_is_allowed(void) {
   FILE *F = tmpfile();
   if (!F)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 060d561c1fe1c2..bd1509f31bc0d0 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -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_fseek(void) {
   FILE *fp = tmpfile();
   fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}
@@ -265,9 +271,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);

[NagyDonat]

I don't see any issue.

[steakhal]

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.

[balazs-benics-sonarsource]

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

[balazske]

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