[clang][analyzer] Add function 'fscanf' to StreamChecker. #78180

balazske · 2024-01-15T16:08:45Z

No description provided.

llvmbot · 2024-01-15T16:09:15Z

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

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

3 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+61)
(modified) clang/test/Analysis/stream-error.c (+25)
(modified) clang/test/Analysis/stream.c (+6)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 95c7503e49e0d32..ab23a428e9397f2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -266,6 +266,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fprintf"}},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
+      {{{"fscanf"}},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFscanf, _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}},
@@ -345,6 +348,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFprintf(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
+  void evalFscanf(const FnDescription *Desc, const CallEvent &Call,
+                  CheckerContext &C) const;
+
   void evalUngetc(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
@@ -975,6 +981,61 @@ void StreamChecker::evalFprintf(const FnDescription *Desc,
   C.addTransition(StateFailed);
 }
 
+void StreamChecker::evalFscanf(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);
+
+  SValBuilder &SVB = C.getSValBuilder();
+  ASTContext &ACtx = C.getASTContext();
+
+  if (OldSS->ErrorState != ErrorFEof) {
+    NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+    ProgramStateRef StateNotFailed =
+        State->BindExpr(CE, C.getLocationContext(), RetVal);
+    auto RetGeZero =
+        SVB.evalBinOp(StateNotFailed, BO_GE, RetVal,
+                      SVB.makeZeroVal(ACtx.IntTy), SVB.getConditionType())
+            .getAs<DefinedOrUnknownSVal>();
+    if (!RetGeZero)
+      return;
+    StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
+
+    C.addTransition(StateNotFailed);
+  }
+
+  // Add transition for the failed state.
+  // Error occurs if nothing is matched yet and reading the input fails.
+  // Error can be EOF, or other error. At "other error" FERROR or 'errno' can
+  // be set but it is not further specified if all are required to be set.
+  // Documentation does not mention, but file position will be set to
+  // indeterminate similarly as at 'fread'.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  StreamErrorState NewES = (OldSS->ErrorState == ErrorFEof)
+                               ? ErrorFEof
+                               : ErrorNone | ErrorFEof | ErrorFError;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
+  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+  if (OldSS->ErrorState != ErrorFEof)
+    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+  else
+    C.addTransition(StateFailed);
+}
+
 void StreamChecker::evalUngetc(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 0f7fdddc0dd4cd7..2cf46e1d4ad51f1 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -208,6 +208,31 @@ void error_fprintf(void) {
   fprintf(F, "ccc"); // expected-warning {{Stream might be already closed}}
 }
 
+void error_fscanf(int *A) {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  int Ret = fscanf(F, "a%ib", A);
+  if (Ret >= 0) {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+    fscanf(F, "bbb");                          // no-warning
+  } else {
+    if (ferror(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fscanf(F, "bbb");               // expected-warning {{might be 'indeterminate'}}
+    } else if (feof(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fscanf(F, "bbb");               // expected-warning {{is in EOF state}}
+      clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+    } else {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fscanf(F, "bbb");               // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+  fscanf(F, "ccc"); // expected-warning {{Stream might be already closed}}
+}
+
 void error_ungetc() {
   FILE *F = tmpfile();
   if (!F)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index e8f06922bdb2f37..36a9b4e26b07a28 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -45,6 +45,12 @@ void check_fprintf(void) {
   fclose(fp);
 }
 
+void check_fscanf(void) {
+  FILE *fp = tmpfile();
+  fscanf(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}}

llvmbot · 2024-01-15T16:09:15Z

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

Author: Balázs Kéri (balazske)

Changes

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

3 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+61)
(modified) clang/test/Analysis/stream-error.c (+25)
(modified) clang/test/Analysis/stream.c (+6)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 95c7503e49e0d32..ab23a428e9397f2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -266,6 +266,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
       {{{"fprintf"}},
        {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
         std::bind(&StreamChecker::evalFprintf, _1, _2, _3, _4), 0}},
+      {{{"fscanf"}},
+       {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+        std::bind(&StreamChecker::evalFscanf, _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}},
@@ -345,6 +348,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFprintf(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
+  void evalFscanf(const FnDescription *Desc, const CallEvent &Call,
+                  CheckerContext &C) const;
+
   void evalUngetc(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
@@ -975,6 +981,61 @@ void StreamChecker::evalFprintf(const FnDescription *Desc,
   C.addTransition(StateFailed);
 }
 
+void StreamChecker::evalFscanf(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);
+
+  SValBuilder &SVB = C.getSValBuilder();
+  ASTContext &ACtx = C.getASTContext();
+
+  if (OldSS->ErrorState != ErrorFEof) {
+    NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+    ProgramStateRef StateNotFailed =
+        State->BindExpr(CE, C.getLocationContext(), RetVal);
+    auto RetGeZero =
+        SVB.evalBinOp(StateNotFailed, BO_GE, RetVal,
+                      SVB.makeZeroVal(ACtx.IntTy), SVB.getConditionType())
+            .getAs<DefinedOrUnknownSVal>();
+    if (!RetGeZero)
+      return;
+    StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
+
+    C.addTransition(StateNotFailed);
+  }
+
+  // Add transition for the failed state.
+  // Error occurs if nothing is matched yet and reading the input fails.
+  // Error can be EOF, or other error. At "other error" FERROR or 'errno' can
+  // be set but it is not further specified if all are required to be set.
+  // Documentation does not mention, but file position will be set to
+  // indeterminate similarly as at 'fread'.
+  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+  StreamErrorState NewES = (OldSS->ErrorState == ErrorFEof)
+                               ? ErrorFEof
+                               : ErrorNone | ErrorFEof | ErrorFError;
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
+  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+  if (OldSS->ErrorState != ErrorFEof)
+    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+  else
+    C.addTransition(StateFailed);
+}
+
 void StreamChecker::evalUngetc(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 0f7fdddc0dd4cd7..2cf46e1d4ad51f1 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -208,6 +208,31 @@ void error_fprintf(void) {
   fprintf(F, "ccc"); // expected-warning {{Stream might be already closed}}
 }
 
+void error_fscanf(int *A) {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  int Ret = fscanf(F, "a%ib", A);
+  if (Ret >= 0) {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+    fscanf(F, "bbb");                          // no-warning
+  } else {
+    if (ferror(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fscanf(F, "bbb");               // expected-warning {{might be 'indeterminate'}}
+    } else if (feof(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fscanf(F, "bbb");               // expected-warning {{is in EOF state}}
+      clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+    } else {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fscanf(F, "bbb");               // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+  fscanf(F, "ccc"); // expected-warning {{Stream might be already closed}}
+}
+
 void error_ungetc() {
   FILE *F = tmpfile();
   if (!F)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index e8f06922bdb2f37..36a9b4e26b07a28 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -45,6 +45,12 @@ void check_fprintf(void) {
   fclose(fp);
 }
 
+void check_fscanf(void) {
+  FILE *fp = tmpfile();
+  fscanf(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}}

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

steakhal · 2024-01-22T10:29:58Z

This patch breaks a downstream test, like this:

void test_fscanf_2() {
  FILE *F1 = tmpfile();
  if (!F1)
    return;

  int a;
  unsigned b;
  fscanf(F1, "%d %u", &a, &b);
  clang_analyzer_dump_int(a); // FP warning: 1st function call argument is an uninitialized value
  fclose(F1);
}

The FP is present, even if I guard the dump with if (ret == 2).

balazske · 2024-01-23T16:08:14Z

This patch breaks a downstream test, like this:

I think this can be caused by missing the default evalCall for fscanf, but did not find the exact reason.

steakhal · 2024-01-24T15:21:32Z

This patch breaks a downstream test, like this:

void test_fscanf_2() {
  FILE *F1 = tmpfile();
  if (!F1)
    return;

  int a;
  unsigned b;
  fscanf(F1, "%d %u", &a, &b);
  clang_analyzer_dump_int(a); // FP warning: 1st function call argument is an uninitialized value
  fclose(F1);
}

The FP is present, even if I guard the dump with if (ret == 2).

I think this can be caused by missing the default evalCall for fscanf, but did not find the exact reason.

Now I know what's going on - after cherry-picking like 15 StreamChecker patches 😅 (Yea, some fun for the last couple of days) Previously the call was default eval called, thus arguments escaped. This is no longer the case, thus the regions &a, &b won't escape, thus preserves their original values (which was UndefinedVal())

This is a regression compared to default eval calling "fscanf".

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

aacfc3f

balazske requested review from steakhal and NagyDonat January 15, 2024 16:08

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Jan 15, 2024

benshi001 reviewed Jan 16, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp Show resolved Hide resolved

added clarifying comment

128b59e

benshi001 approved these changes Jan 17, 2024

View reviewed changes

balazske mentioned this pull request Jan 19, 2024

[clang][analyzer] Support 'getdelim' and 'getline' in StreamChecker #78693

Merged

NagyDonat approved these changes Jan 19, 2024

View reviewed changes

balazske merged commit 0845514 into llvm:main Jan 22, 2024
4 checks passed

balazske deleted the streamchecker_fscanf branch January 22, 2024 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][analyzer] Add function 'fscanf' to StreamChecker. #78180

[clang][analyzer] Add function 'fscanf' to StreamChecker. #78180

balazske commented Jan 15, 2024

llvmbot commented Jan 15, 2024

llvmbot commented Jan 15, 2024

steakhal commented Jan 22, 2024

balazske commented Jan 23, 2024

steakhal commented Jan 24, 2024

[clang][analyzer] Add function 'fscanf' to StreamChecker. #78180

[clang][analyzer] Add function 'fscanf' to StreamChecker. #78180

Conversation

balazske commented Jan 15, 2024

llvmbot commented Jan 15, 2024

llvmbot commented Jan 15, 2024

steakhal commented Jan 22, 2024

balazske commented Jan 23, 2024

steakhal commented Jan 24, 2024