diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp index 6b8ac2629453d4..6cc88679458140 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -2388,12 +2388,15 @@ void StdLibraryFunctionsChecker::initFunctionSummaries( .ArgConstraint(NotNull(ArgNo(0)))); // int fileno(FILE *stream); + // According to POSIX 'fileno' may fail and set 'errno'. + // But in Linux it may fail only if the specified file pointer is invalid. + // At many places 'fileno' is used without check for failure and a failure + // case here would produce a large amount of likely false positive warnings. + // To avoid this, we assume here that it does not fail. addToFunctionSummaryMap( "fileno", Signature(ArgTypes{FilePtrTy}, RetType{IntTy}), Summary(NoEvalCall) - .Case(ReturnsValidFileDescriptor, ErrnoMustNotBeChecked, - GenericSuccessMsg) - .Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg) + .Case(ReturnsValidFileDescriptor, ErrnoUnchanged, GenericSuccessMsg) .ArgConstraint(NotNull(ArgNo(0)))); // void rewind(FILE *stream); diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 7e7e3f0eee2b48..a070f451694a3b 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -249,6 +249,10 @@ struct StreamOperationEvaluator { bool isStreamEof() const { return SS->ErrorState == ErrorFEof; } + NonLoc getZeroVal(const CallEvent &Call) { + return *SVB.makeZeroVal(Call.getResultType()).getAs(); + } + ProgramStateRef setStreamState(ProgramStateRef State, const StreamState &NewSS) { return State->set(StreamSym, NewSS); @@ -390,7 +394,8 @@ class StreamChecker : public Checker FnTestDescriptions = { @@ -486,6 +491,9 @@ class StreamChecker : public CheckerBindExpr(E.CE, C.getLocationContext(), RetVal); StateNotFailed = - E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, - *E.SVB.makeZeroVal(E.ACtx.IntTy).getAs()); + E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call)); if (!StateNotFailed) return; StateNotFailed = @@ -1003,8 +1010,7 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call, ProgramStateRef StateNotFailed = State->BindExpr(E.CE, C.getLocationContext(), RetVal); StateNotFailed = - E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, - *E.SVB.makeZeroVal(E.ACtx.IntTy).getAs()); + E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call)); if (StateNotFailed) C.addTransition(StateNotFailed); } @@ -1073,8 +1079,7 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc, ProgramStateRef StateNotFailed = State->BindExpr(E.CE, C.getLocationContext(), RetVal); StateNotFailed = - E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, - *E.SVB.makeZeroVal(E.CE->getType()).getAs()); + E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call)); if (!StateNotFailed) return; C.addTransition(StateNotFailed); @@ -1200,8 +1205,7 @@ void StreamChecker::evalFtell(const FnDescription *Desc, const CallEvent &Call, ProgramStateRef StateNotFailed = State->BindExpr(E.CE, C.getLocationContext(), RetVal); StateNotFailed = - E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, - *E.SVB.makeZeroVal(Call.getResultType()).getAs()); + E.assumeBinOpNN(StateNotFailed, BO_GE, RetVal, E.getZeroVal(Call)); if (!StateNotFailed) return; @@ -1226,79 +1230,6 @@ void StreamChecker::evalRewind(const FnDescription *Desc, const CallEvent &Call, C.addTransition(State); } -void StreamChecker::evalClearerr(const FnDescription *Desc, - const CallEvent &Call, - CheckerContext &C) const { - ProgramStateRef State = C.getState(); - StreamOperationEvaluator E(C); - if (!E.Init(Desc, Call, C, State)) - return; - - // FilePositionIndeterminate is not cleared. - State = E.setStreamState( - State, - StreamState::getOpened(Desc, ErrorNone, E.SS->FilePositionIndeterminate)); - C.addTransition(State); -} - -void StreamChecker::evalFeofFerror(const FnDescription *Desc, - const CallEvent &Call, CheckerContext &C, - const StreamErrorState &ErrorKind) const { - ProgramStateRef State = C.getState(); - StreamOperationEvaluator E(C); - if (!E.Init(Desc, Call, C, State)) - return; - - if (E.SS->ErrorState & ErrorKind) { - // Execution path with error of ErrorKind. - // Function returns true. - // From now on it is the only one error state. - ProgramStateRef TrueState = bindAndAssumeTrue(State, C, E.CE); - C.addTransition(E.setStreamState( - TrueState, StreamState::getOpened(Desc, ErrorKind, - E.SS->FilePositionIndeterminate && - !ErrorKind.isFEof()))); - } - if (StreamErrorState NewES = E.SS->ErrorState & (~ErrorKind)) { - // Execution path(s) with ErrorKind not set. - // Function returns false. - // New error state is everything before minus ErrorKind. - ProgramStateRef FalseState = E.bindReturnValue(State, C, 0); - C.addTransition(E.setStreamState( - FalseState, - StreamState::getOpened( - Desc, NewES, E.SS->FilePositionIndeterminate && !NewES.isFEof()))); - } -} - -void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call, - CheckerContext &C) const { - ProgramStateRef State = C.getState(); - SVal StreamVal = getStreamArg(Desc, Call); - State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C, - State); - if (!State) - return; - State = ensureStreamOpened(StreamVal, C, State); - if (!State) - return; - - C.addTransition(State); -} - -void StreamChecker::evalSetFeofFerror(const FnDescription *Desc, - const CallEvent &Call, CheckerContext &C, - const StreamErrorState &ErrorKind) const { - ProgramStateRef State = C.getState(); - SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol(); - assert(StreamSym && "Operation not permitted on non-symbolic stream value."); - const StreamState *SS = State->get(StreamSym); - assert(SS && "Stream should be tracked by the checker."); - State = State->set( - StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind)); - C.addTransition(State); -} - void StreamChecker::preFflush(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const { ProgramStateRef State = C.getState(); @@ -1377,6 +1308,104 @@ void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent &Call, C.addTransition(StateFailed); } +void StreamChecker::evalClearerr(const FnDescription *Desc, + const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + StreamOperationEvaluator E(C); + if (!E.Init(Desc, Call, C, State)) + return; + + // FilePositionIndeterminate is not cleared. + State = E.setStreamState( + State, + StreamState::getOpened(Desc, ErrorNone, E.SS->FilePositionIndeterminate)); + C.addTransition(State); +} + +void StreamChecker::evalFeofFerror(const FnDescription *Desc, + const CallEvent &Call, CheckerContext &C, + const StreamErrorState &ErrorKind) const { + ProgramStateRef State = C.getState(); + StreamOperationEvaluator E(C); + if (!E.Init(Desc, Call, C, State)) + return; + + if (E.SS->ErrorState & ErrorKind) { + // Execution path with error of ErrorKind. + // Function returns true. + // From now on it is the only one error state. + ProgramStateRef TrueState = bindAndAssumeTrue(State, C, E.CE); + C.addTransition(E.setStreamState( + TrueState, StreamState::getOpened(Desc, ErrorKind, + E.SS->FilePositionIndeterminate && + !ErrorKind.isFEof()))); + } + if (StreamErrorState NewES = E.SS->ErrorState & (~ErrorKind)) { + // Execution path(s) with ErrorKind not set. + // Function returns false. + // New error state is everything before minus ErrorKind. + ProgramStateRef FalseState = E.bindReturnValue(State, C, 0); + C.addTransition(E.setStreamState( + FalseState, + StreamState::getOpened( + Desc, NewES, E.SS->FilePositionIndeterminate && !NewES.isFEof()))); + } +} + +void StreamChecker::evalFileno(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) const { + // Fileno should fail only if the passed pointer is invalid. + // Some of the preconditions are checked already in preDefault. + // Here we can assume that the operation does not fail, because if we + // introduced a separate branch where fileno() returns -1, then it would cause + // many unexpected and unwanted warnings in situations where fileno() is + // called on valid streams. + // The stream error states are not modified by 'fileno', and 'errno' is also + // left unchanged (so this evalCall does not invalidate it, but we have a + // custom evalCall instead of the default that would invalidate it). + ProgramStateRef State = C.getState(); + StreamOperationEvaluator E(C); + if (!E.Init(Desc, Call, C, State)) + return; + + NonLoc RetVal = makeRetVal(C, E.CE).castAs(); + State = State->BindExpr(E.CE, C.getLocationContext(), RetVal); + State = E.assumeBinOpNN(State, BO_GE, RetVal, E.getZeroVal(Call)); + if (!State) + return; + + C.addTransition(State); +} + +void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + SVal StreamVal = getStreamArg(Desc, Call); + State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C, + State); + if (!State) + return; + State = ensureStreamOpened(StreamVal, C, State); + if (!State) + return; + + C.addTransition(State); +} + +void StreamChecker::evalSetFeofFerror(const FnDescription *Desc, + const CallEvent &Call, CheckerContext &C, + const StreamErrorState &ErrorKind) const { + ProgramStateRef State = C.getState(); + SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol(); + assert(StreamSym && "Operation not permitted on non-symbolic stream value."); + const StreamState *SS = State->get(StreamSym); + assert(SS && "Stream should be tracked by the checker."); + State = State->set( + StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind)); + C.addTransition(State); +} + ProgramStateRef StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE, CheckerContext &C, diff --git a/clang/test/Analysis/std-c-library-functions-path-notes.c b/clang/test/Analysis/std-c-library-functions-path-notes.c index 4df00fe1e60646..6449b71928fa7c 100644 --- a/clang/test/Analysis/std-c-library-functions-path-notes.c +++ b/clang/test/Analysis/std-c-library-functions-path-notes.c @@ -61,24 +61,22 @@ int test_islower(int *x) { } int test_bugpath_notes(FILE *f1, char c, FILE *f2) { - int f = fileno(f2); - if (f == -1) // \ + // This test has the purpose of checking that notes appear at correct place. + long a = ftell(f2); // no note + if (a == -1) // \ // expected-note{{Taking false branch}} - return 0; - int l = islower(c); - f = fileno(f1); // \ - // expected-note{{Value assigned to 'f'}} \ - // expected-note{{Assuming that 'fileno' fails}} - return dup(f); // \ + return -1; + int l = islower(c); // no note + a = ftell(f1); // \ + // expected-note{{Value assigned to 'a'}} \ + // expected-note{{Assuming that 'ftell' fails}} + return dup(a); // \ // expected-warning{{The 1st argument to 'dup' is -1 but should be >= 0}} \ // expected-note{{The 1st argument to 'dup' is -1 but should be >= 0}} } int test_fileno_arg_note(FILE *f1) { - return dup(fileno(f1)); // \ - // expected-warning{{The 1st argument to 'dup' is < 0 but should be >= 0}} \ - // expected-note{{The 1st argument to 'dup' is < 0 but should be >= 0}} \ - // expected-note{{Assuming that 'fileno' fails}} + return dup(fileno(f1)); // no warning } int test_readlink_bufsize_zero(char *Buf, size_t Bufsize) { diff --git a/clang/test/Analysis/stream-errno-note.c b/clang/test/Analysis/stream-errno-note.c index 2531e26e200385..2411a2d9a00a72 100644 --- a/clang/test/Analysis/stream-errno-note.c +++ b/clang/test/Analysis/stream-errno-note.c @@ -141,16 +141,8 @@ void check_rewind_errnocheck(void) { } void check_fileno(void) { - FILE *F = tmpfile(); - // expected-note@+2{{'F' is non-null}} - // expected-note@+1{{Taking false branch}} - if (!F) - return; - fileno(F); - // expected-note@-1{{Assuming that 'fileno' is successful; 'errno' becomes undefined after the call}} - if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}} - // expected-note@-1{{An undefined value may be read from 'errno'}} - (void)fclose(F); + // nothing to check: checker assumes that 'fileno' is always successful + // (and does not change 'errno') } void check_fwrite_zeroarg(size_t Siz) { diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c index fab6a58b3275a8..5f0a58032fa263 100644 --- a/clang/test/Analysis/stream-errno.c +++ b/clang/test/Analysis/stream-errno.c @@ -173,6 +173,8 @@ void check_no_errno_change(void) { if (errno) {} // no-warning ferror(F); if (errno) {} // no-warning + fileno(F); + if (errno) {} // no-warning clang_analyzer_eval(errno == 1); // expected-warning{{TRUE}} fclose(F); } @@ -250,20 +252,6 @@ void check_rewind(void) { fclose(F); } -void check_fileno(void) { - FILE *F = tmpfile(); - if (!F) - return; - int N = fileno(F); - if (N == -1) { - clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} - if (errno) {} // no-warning - fclose(F); - return; - } - if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}} -} - void check_fflush_opened_file(void) { FILE *F = tmpfile(); if (!F) diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c index 4bab07577ccd53..ac31083bfc691f 100644 --- a/clang/test/Analysis/stream-error.c +++ b/clang/test/Analysis/stream-error.c @@ -491,6 +491,24 @@ void error_ftello(void) { fclose(F); } +void error_fileno(void) { + FILE *F = fopen("file", "r"); + if (!F) + return; + int N = fileno(F); + clang_analyzer_eval(N >= 0); // expected-warning {{TRUE}} + clang_analyzer_eval(feof(F) && ferror(F)); // expected-warning {{FALSE}} + StreamTesterChecker_make_feof_stream(F); + N = fileno(F); + clang_analyzer_eval(feof(F)); // expected-warning {{TRUE}} + clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}} + StreamTesterChecker_make_ferror_stream(F); + N = fileno(F); + clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}} + clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}} + fclose(F); +} + void error_fflush_on_non_null_stream_clear_error_states(void) { FILE *F0 = tmpfile(), *F1 = tmpfile(); // `fflush` clears a non-EOF stream's error state. diff --git a/clang/test/Analysis/stream-noopen.c b/clang/test/Analysis/stream-noopen.c index 8bd01a90cf8596..644c699d05e246 100644 --- a/clang/test/Analysis/stream-noopen.c +++ b/clang/test/Analysis/stream-noopen.c @@ -268,6 +268,16 @@ void test_clearerr(FILE *F) { // expected-warning@-1{{FALSE}} } +void test_fileno(FILE *F) { + errno = 0; + int A = fileno(F); + clang_analyzer_eval(F != NULL); // expected-warning{{TRUE}} + clang_analyzer_eval(A >= 0); // expected-warning{{TRUE}} + if (errno) {} // no-warning + clang_analyzer_eval(errno == 0); // expected-warning{{TRUE}} + // expected-warning@-1{{FALSE}} +} + void freadwrite_zerosize(FILE *F) { fwrite(WBuf, 1, 0, F); clang_analyzer_eval(feof(F)); // expected-warning {{UNKNOWN}}