From 47df664c7acfd8abd082c0252d1793182c92dc3d Mon Sep 17 00:00:00 2001 From: Ben Shi <2283975856@qq.com> Date: Wed, 29 Nov 2023 19:20:49 +0800 Subject: [PATCH] [clang][analyzer] Support `fgets` in the SteamChecker (#73638) --- .../StaticAnalyzer/Checkers/StreamChecker.cpp | 94 ++++++++++++------- .../Analysis/Inputs/system-header-simulator.h | 1 + clang/test/Analysis/stream-error.c | 23 +++++ clang/test/Analysis/stream.c | 7 ++ 4 files changed, 89 insertions(+), 36 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index eccb2063fad10d..a4799b5f762cae 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -252,7 +252,10 @@ class StreamChecker : public CheckerErrorState != ErrorFEof) { - NonLoc RetVal = makeRetVal(C, CE).castAs(); - 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(); - auto CondHigh = - SVB.evalBinOp(State, BO_LE, RetVal, - SVB.makeIntVal(SVB.getBasicValueFactory() - .getMaxValue(ASTC.UnsignedCharTy) - .getLimitedValue(), - ASTC.IntTy), - SVB.getConditionType()) - .getAs(); - 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(); + 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(); + auto CondHigh = + SVB.evalBinOp(State, BO_LE, RetVal, + SVB.makeIntVal(SVB.getBasicValueFactory() + .getMaxValue(ASTC.UnsignedCharTy) + .getLimitedValue(), + ASTC.IntTy), + SVB.getConditionType()) + .getAs(); + 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 GetBuf = + Call.getArgSVal(0).getAs(); + if (!GetBuf) + return; + ProgramStateRef StateNotFailed = + State->BindExpr(CE, C.getLocationContext(), *GetBuf); + StateNotFailed = StateNotFailed->set( + 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 index 7ef5f29fbf42cb..7089bd8bfc9d98 100644 --- 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 index c90d0cdc5742d3..c8332bcbfa8ca7 100644 --- 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 index b0f39af8eb2f3a..0b655dbdc571fe 100644 --- 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}}