diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -207,6 +207,10 @@ Static Analyzer --------------- +- Checker ``alpha.unix.Stream`` is improved to be able to detect bugs related + to bad ``errno`` usage. The effects are visible if checker + ``alpha.unix.Errno`` is turned on additionally. + .. _release-notes-ubsan: Undefined Behavior Sanitizer (UBSan) diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -446,9 +446,10 @@ } }; - /// Set errno constraints if use of 'errno' is irrelevant to the - /// modeled function or modeling is not possible. - class NoErrnoConstraint : public ErrnoConstraintBase { + /// Reset errno constraints to irrelevant. + /// This is applicable to functions that may change 'errno' and are not + /// modeled elsewhere. + class ResetErrnoConstraint : public ErrnoConstraintBase { public: ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary, @@ -457,6 +458,19 @@ } }; + /// Do not change errno constraints. + /// This is applicable to functions that are modeled in another checker + /// and the already set errno constraints should not be changed in the + /// post-call event. + class NoErrnoConstraint : public ErrnoConstraintBase { + public: + ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, + const Summary &Summary, + CheckerContext &C) const override { + return State; + } + }; + /// A single branch of a function summary. /// /// A branch is defined by a series of constraints - "assumptions" - @@ -716,7 +730,8 @@ /// Usually if a failure return value exists for function, that function /// needs different cases for success and failure with different errno /// constraints (and different return value constraints). - const NoErrnoConstraint ErrnoIrrelevant{}; + const NoErrnoConstraint ErrnoAlreadySet{}; + const ResetErrnoConstraint ErrnoIrrelevant{}; const SuccessErrnoConstraint ErrnoMustNotBeChecked{}; const FailureErrnoConstraint ErrnoNEZeroIrrelevant{}; }; @@ -1555,7 +1570,7 @@ Summary(NoEvalCall) .Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)), ReturnValueCondition(WithinRange, Range(0, SizeMax))}, - ErrnoIrrelevant) + ErrnoAlreadySet) .ArgConstraint(NotNull(ArgNo(0))) .ArgConstraint(NotNull(ArgNo(3))) .ArgConstraint(BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1), diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -10,6 +10,7 @@ // //===----------------------------------------------------------------------===// +#include "ErrnoModeling.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -17,6 +18,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" @@ -278,6 +280,8 @@ 0}}, }; + mutable Optional EofVal; + void evalFopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; @@ -411,6 +415,26 @@ }); } + /// Get a SVal to be used as new errno value at failure cases of stream + /// functions. + NonLoc getErrnoSVal(CheckerContext &C, const CallEvent &Call) const { + return C.getSValBuilder() + .conjureSymbolVal(this, Call.getOriginExpr(), C.getLocationContext(), + C.getASTContext().IntTy, C.blockCount()) + .castAs(); + } + + void initEof(CheckerContext &C) const { + if (EofVal) + return; + + if (const llvm::Optional OptInt = + tryExpandAsInteger("EOF", C.getPreprocessor())) + EofVal = *OptInt; + else + EofVal = -1; + } + /// Searches for the ExplodedNode where the file descriptor was acquired for /// StreamSym. static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N, @@ -457,6 +481,8 @@ void StreamChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { + initEof(C); + const FnDescription *Desc = lookupFn(Call); if (!Desc || !Desc->PreFn) return; @@ -497,11 +523,18 @@ StateNotNull = StateNotNull->set(RetSym, StreamState::getOpened(Desc)); + StateNotNull = errno_modeling::setErrnoForStdSuccess(StateNotNull, C); + StateNull = StateNull->set(RetSym, StreamState::getOpenFailed(Desc)); - - C.addTransition(StateNotNull, - constructNoteTag(C, RetSym, "Stream opened here")); + StateNull = errno_modeling::setErrnoForStdFailure(StateNull, C, + getErrnoSVal(C, Call)); + + ExplodedNode *N = C.addTransition( + StateNotNull, constructNoteTag(C, RetSym, "Stream opened here")); + C.addTransition(N->getState(), N, + errno_modeling::getNoteTagForStdSuccess( + C, cast(Call.getDecl())->getNameAsString())); C.addTransition(StateNull); } @@ -555,11 +588,17 @@ StateRetNotNull = StateRetNotNull->set(StreamSym, StreamState::getOpened(Desc)); + StateRetNotNull = errno_modeling::setErrnoForStdSuccess(StateRetNotNull, C); + StateRetNull = StateRetNull->set(StreamSym, StreamState::getOpenFailed(Desc)); + StateRetNull = errno_modeling::setErrnoForStdFailure(StateRetNull, C, + getErrnoSVal(C, Call)); - C.addTransition(StateRetNotNull, - constructNoteTag(C, StreamSym, "Stream reopened here")); + ExplodedNode *N = C.addTransition( + StateRetNotNull, constructNoteTag(C, StreamSym, "Stream reopened here")); + C.addTransition(N->getState(), N, + errno_modeling::getNoteTagForStdSuccess(C, "freopen")); C.addTransition(StateRetNull); } @@ -574,6 +613,10 @@ if (!SS) return; + auto *CE = dyn_cast_or_null(Call.getOriginExpr()); + if (!CE) + return; + assertStreamStateOpened(SS); // Close the File Descriptor. @@ -581,7 +624,21 @@ // and can not be used any more. State = State->set(Sym, StreamState::getClosed(Desc)); - C.addTransition(State); + // Return 0 on success, EOF on failure. + SValBuilder &SVB = C.getSValBuilder(); + ProgramStateRef StateSuccess = State->BindExpr( + CE, C.getLocationContext(), SVB.makeIntVal(0, C.getASTContext().IntTy)); + ProgramStateRef StateFailure = + State->BindExpr(CE, C.getLocationContext(), + SVB.makeIntVal(*EofVal, C.getASTContext().IntTy)); + + StateSuccess = errno_modeling::setErrnoForStdSuccess(StateSuccess, C); + StateFailure = errno_modeling::setErrnoForStdFailure(StateFailure, C, + getErrnoSVal(C, Call)); + + C.addTransition(StateSuccess, + errno_modeling::getNoteTagForStdSuccess(C, "fclose")); + C.addTransition(StateFailure); } void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call, @@ -663,6 +720,8 @@ // This is the "size or nmemb is zero" case. // Just return 0, do nothing more (not clear the error flags). State = bindInt(0, State, C, CE); + // It is unspecified what can happen with 'errno'. + State = errno_modeling::setErrnoState(State, errno_modeling::Irrelevant); C.addTransition(State); return; } @@ -674,7 +733,10 @@ State->BindExpr(CE, C.getLocationContext(), *NMembVal); StateNotFailed = StateNotFailed->set(StreamSym, StreamState::getOpened(Desc)); - C.addTransition(StateNotFailed); + StateNotFailed = errno_modeling::setErrnoForStdSuccess(StateNotFailed, C); + C.addTransition(StateNotFailed, + errno_modeling::getNoteTagForStdSuccess( + C, cast(Call.getDecl())->getNameAsString())); } // Add transition for the failed state. @@ -701,6 +763,8 @@ // indicator for the stream is indeterminate. StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof()); StateFailed = StateFailed->set(StreamSym, NewSS); + StateFailed = errno_modeling::setErrnoForStdFailure(StateFailed, C, + getErrnoSVal(C, Call)); if (IsFread && OldSS->ErrorState != ErrorFEof) C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); else @@ -762,7 +826,12 @@ StreamSym, StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true)); - C.addTransition(StateNotFailed); + StateNotFailed = errno_modeling::setErrnoForStdSuccess(StateNotFailed, C); + StateFailed = errno_modeling::setErrnoForStdFailure(StateFailed, C, + getErrnoSVal(C, Call)); + + C.addTransition(StateNotFailed, + errno_modeling::getNoteTagForStdSuccess(C, "fseek")); C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym)); } @@ -780,6 +849,8 @@ assertStreamStateOpened(SS); + // According to POSIX no change to 'errno' shall happen. + // FilePositionIndeterminate is not cleared. State = State->set( StreamSym, @@ -805,6 +876,8 @@ assertStreamStateOpened(SS); + // According to POSIX no change to 'errno' shall happen. + if (SS->ErrorState & ErrorKind) { // Execution path with error of ErrorKind. // Function returns true. diff --git a/clang/test/Analysis/stream-errno-note.c b/clang/test/Analysis/stream-errno-note.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/stream-errno-note.c @@ -0,0 +1,99 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-output text -verify %s + +#include "Inputs/system-header-simulator.h" +#include "Inputs/errno_func.h" + +void check_fopen(void) { + FILE *F = fopen("xxx", "r"); + // expected-note@-1{{Assuming that function 'fopen' is successful, in this case the value 'errno' may be undefined after the call and should not be used}} + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} + // expected-note@-1{{An undefined value may be read from 'errno'}} + fclose(F); +} + +void check_tmpfile(void) { + FILE *F = tmpfile(); + // expected-note@-1{{Assuming that function 'tmpfile' is successful, in this case the value 'errno' may be undefined after the call and should not be used}} + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} + // expected-note@-1{{An undefined value may be read from 'errno'}} + fclose(F); +} + +void check_freopen(void) { + FILE *F = tmpfile(); + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + F = freopen("xxx", "w", F); + // expected-note@-1{{Assuming that function 'freopen' is successful}} + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}} + // expected-note@-1{{An undefined value may be read from 'errno'}} + fclose(F); +} + +void check_fclose(void) { + FILE *F = tmpfile(); + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + (void)fclose(F); + // expected-note@-1{{Assuming that function 'fclose' is successful}} + if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}} + // expected-note@-1{{An undefined value may be read from 'errno'}} +} + +void check_fread(void) { + char Buf[10]; + FILE *F = tmpfile(); + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + (void)fread(Buf, 1, 10, F); + // expected-note@-1{{Assuming that function 'fread' is successful}} + 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); +} + +void check_fwrite(void) { + char Buf[] = "0123456789"; + FILE *F = tmpfile(); + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + int R = fwrite(Buf, 1, 10, F); + // expected-note@-1{{Assuming that function 'fwrite' is successful}} + 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); +} + +void check_fseek(void) { + FILE *F = tmpfile(); + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + (void)fseek(F, 11, SEEK_SET); + // expected-note@-1{{Assuming that function 'fseek' is successful}} + 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); +} diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/stream-errno.c @@ -0,0 +1,123 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,apiModeling.StdCLibraryFunctions,debug.ExprInspection \ +// RUN: -verify %s + +#include "Inputs/system-header-simulator.h" +#include "Inputs/errno_func.h" + +extern void clang_analyzer_eval(int); +extern void clang_analyzer_dump(int); + +void check_fopen(void) { + FILE *F = fopen("xxx", "r"); + if (!F) { + clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} + if (errno) {} // no-warning + return; + } + if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} +} + +void check_tmpfile(void) { + FILE *F = tmpfile(); + if (!F) { + clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} + if (errno) {} // no-warning + return; + } + if (errno) {} // expected-warning{{An undefined value may be read from 'errno' [alpha.unix.Errno]}} +} + +void check_freopen(void) { + FILE *F = tmpfile(); + if (!F) + return; + F = freopen("xxx", "w", F); + if (!F) { + clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} + if (errno) {} // no-warning + return; + } + if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}} +} + +void check_fclose(void) { + FILE *F = tmpfile(); + if (!F) + return; + int Ret = fclose(F); + if (Ret == EOF) { + clang_analyzer_eval(errno != 0); // expected-warning{{TRUE}} + if (errno) {} // no-warning + return; + } + if (errno) {} // expected-warning{{An undefined value may be read from 'errno'}} +} + +void check_fread(void) { + char Buf[10]; + FILE *F = tmpfile(); + if (!F) + return; + fread(Buf, 0, 1, F); + if (errno) {} // no-warning + fread(Buf, 1, 0, F); + if (errno) {} // no-warning + + int R = fread(Buf, 1, 10, F); + if (R < 10) { + 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_fwrite(void) { + char Buf[] = "0123456789"; + FILE *F = tmpfile(); + if (!F) + return; + fwrite(Buf, 0, 1, F); + if (errno) {} // no-warning + fwrite(Buf, 1, 0, F); + if (errno) {} // no-warning + + int R = fwrite(Buf, 1, 10, F); + if (R < 10) { + 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_fseek(void) { + FILE *F = tmpfile(); + if (!F) + return; + int S = fseek(F, 11, SEEK_SET); + if (S != 0) { + 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_no_errno_change(void) { + FILE *F = tmpfile(); + if (!F) + return; + errno = 1; + clearerr(F); + if (errno) {} // no-warning + feof(F); + if (errno) {} // no-warning + ferror(F); + if (errno) {} // no-warning + clang_analyzer_eval(errno == 1); // expected-warning{{TRUE}} + fclose(F); +}