[clang][analyzer] Support fgets in the SteamChecker (llvm#73638)

ROCm · Nov 29, 2023 · 47df664 · 47df664
1 parent 1bfb84b
commit 47df664
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 36 deletions.
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -252,7 +252,10 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
  std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
  {{{"fgetc"}, 1},
  {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
- std::bind(&StreamChecker::evalFgetc, _1, _2, _3, _4), 0}},
+ std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, true), 0}},
+ {{{"fgets"}, 3},
+ {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, true),
+ std::bind(&StreamChecker::evalFgetx, _1, _2, _3, _4, false), 2}},
  {{{"fputc"}, 2},
  {std::bind(&StreamChecker::preReadWrite, _1, _2, _3, _4, false),
  std::bind(&StreamChecker::evalFputx, _1, _2, _3, _4, true), 1}},
@@ -320,8 +323,8 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
  void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
  CheckerContext &C, bool IsFread) const;
 
- void evalFgetc(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const;
+ void evalFgetx(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C, bool SingleChar) const;
 
  void evalFputx(const FnDescription *Desc, const CallEvent &Call,
  CheckerContext &C, bool IsSingleChar) const;
@@ -760,8 +763,8 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
  C.addTransition(StateFailed);
 }
 
-void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const {
+void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
+ CheckerContext &C, bool SingleChar) const {
  ProgramStateRef State = C.getState();
  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
  if (!StreamSym)
@@ -778,42 +781,61 @@ void StreamChecker::evalFgetc(const FnDescription *Desc, const CallEvent &Call,
  assertStreamStateOpened(OldSS);
 
  // `fgetc` returns the read character on success, otherwise returns EOF.
+ // `fgets` returns the read buffer address on success, otherwise returns NULL.
 
- // Generate a transition for the success state of `fgetc`.
- // If we know the state to be FEOF at fgetc, do not add a success state.
  if (OldSS->ErrorState != ErrorFEof) {
- NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
- ProgramStateRef StateNotFailed =
- State->BindExpr(CE, C.getLocationContext(), RetVal);
- SValBuilder &SVB = C.getSValBuilder();
- ASTContext &ASTC = C.getASTContext();
- // The returned 'unsigned char' of `fgetc` is converted to 'int',
- // so we need to check if it is in range [0, 255].
- auto CondLow =
- SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
- SVB.getConditionType())
- .getAs<DefinedOrUnknownSVal>();
- auto CondHigh =
- SVB.evalBinOp(State, BO_LE, RetVal,
- SVB.makeIntVal(SVB.getBasicValueFactory()
- .getMaxValue(ASTC.UnsignedCharTy)
- .getLimitedValue(),
- ASTC.IntTy),
- SVB.getConditionType())
- .getAs<DefinedOrUnknownSVal>();
- if (!CondLow || !CondHigh)
- return;
- StateNotFailed = StateNotFailed->assume(*CondLow, true);
- if (!StateNotFailed)
- return;
- StateNotFailed = StateNotFailed->assume(*CondHigh, true);
- if (!StateNotFailed)
- return;
- C.addTransition(StateNotFailed);
+ if (SingleChar) {
+ // Generate a transition for the success state of `fgetc`.
+ NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+ ProgramStateRef StateNotFailed =
+ State->BindExpr(CE, C.getLocationContext(), RetVal);
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ASTC = C.getASTContext();
+ // The returned 'unsigned char' of `fgetc` is converted to 'int',
+ // so we need to check if it is in range [0, 255].
+ auto CondLow =
+ SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
+ SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ auto CondHigh =
+ SVB.evalBinOp(State, BO_LE, RetVal,
+ SVB.makeIntVal(SVB.getBasicValueFactory()
+ .getMaxValue(ASTC.UnsignedCharTy)
+ .getLimitedValue(),
+ ASTC.IntTy),
+ SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!CondLow || !CondHigh)
+ return;
+ StateNotFailed = StateNotFailed->assume(*CondLow, true);
+ if (!StateNotFailed)
+ return;
+ StateNotFailed = StateNotFailed->assume(*CondHigh, true);
+ if (!StateNotFailed)
+ return;
+ C.addTransition(StateNotFailed);
+ } else {
+ // Generate a transition for the success state of `fgets`.
+ std::optional<DefinedSVal> GetBuf =
+ Call.getArgSVal(0).getAs<DefinedSVal>();
+ if (!GetBuf)
+ return;
+ ProgramStateRef StateNotFailed =
+ State->BindExpr(CE, C.getLocationContext(), *GetBuf);
+ StateNotFailed = StateNotFailed->set<StreamMap>(
+ StreamSym, StreamState::getOpened(Desc));
+ C.addTransition(StateNotFailed);
+ }
  }
 
  // Add transition for the failed state.
- ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
+ ProgramStateRef StateFailed;
+ if (SingleChar)
+ StateFailed = bindInt(*EofVal, State, C, CE);
+ else
+ StateFailed =
+ State->BindExpr(CE, C.getLocationContext(),
+ C.getSValBuilder().makeNullWithType(CE->getType()));
 
  // If a (non-EOF) error occurs, the resulting value of the file position
  // indicator for the stream is indeterminate.

diff --git a/clang/test/Analysis/Inputs/system-header-simulator.h b/clang/test/Analysis/Inputs/system-header-simulator.h
@@ -49,6 +49,7 @@ int fclose(FILE *fp);
 size_t fread(void *restrict, size_t, size_t, FILE *restrict);
 size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
 int fgetc(FILE *stream);
+char *fgets(char *restrict str, int count, FILE *restrict stream);
 int fputc(int ch, FILE *stream);
 int fputs(const char *restrict s, FILE *restrict stream);
 int fseek(FILE *__stream, long int __off, int __whence);

diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
@@ -123,6 +123,29 @@ void error_fgetc(void) {
  fgetc(F); // expected-warning {{Stream might be already closed}}
 }
 
+void error_fgets(void) {
+ FILE *F = tmpfile();
+ char Buf[256];
+ if (!F)
+ return;
+ char *Ret = fgets(Buf, sizeof(Buf), F);
+ if (Ret == Buf) {
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+ } else {
+ clang_analyzer_eval(Ret == NULL); // expected-warning {{TRUE}}
+ clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+ if (feof(F)) {
+ clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{Read function called when stream is in EOF state}}
+ } else {
+ clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{might be 'indeterminate'}}
+ }
+ }
+ fclose(F);
+ fgets(Buf, sizeof(Buf), F); // expected-warning {{Stream might be already closed}}
+}
+
 void error_fputc(void) {
  FILE *F = tmpfile();
  if (!F)

diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
@@ -20,6 +20,13 @@ void check_fgetc(void) {
  fclose(fp);
 }
 
+void check_fgets(void) {
+ FILE *fp = tmpfile();
+ char buf[256];
+ fgets(buf, sizeof(buf), fp); // expected-warning {{Stream pointer might be NULL}}
+ fclose(fp);
+}
+
 void check_fputc(void) {
  FILE *fp = tmpfile();
  fputc('A', fp); // expected-warning {{Stream pointer might be NULL}}