diff --git a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp index be2fa91b994a2..1b34ea0e056e5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp @@ -312,18 +312,6 @@ ProgramStateRef setErrnoStdMustBeChecked(ProgramStateRef State, return setErrnoState(State, MustBeChecked); } -const NoteTag *getNoteTagForStdSuccess(CheckerContext &C, llvm::StringRef Fn) { - return getErrnoNoteTag( - C, llvm::formatv( - "'errno' may be undefined after successful call to '{0}'", Fn)); -} - -const NoteTag *getNoteTagForStdMustBeChecked(CheckerContext &C, - llvm::StringRef Fn) { - return getErrnoNoteTag( - C, llvm::formatv("'{0}' indicates failure only by setting 'errno'", Fn)); -} - } // namespace errno_modeling } // namespace ento } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h index 0707fd16d6e60..6b53572fe5e2d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h +++ b/clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h @@ -84,8 +84,7 @@ const NoteTag *getErrnoNoteTag(CheckerContext &C, const std::string &Message); /// Set errno state for the common case when a standard function is successful. /// Set \c ErrnoCheckState to \c MustNotBeChecked (the \c errno value is not -/// affected). At the state transition a note tag created by -/// \c getNoteTagForStdSuccess can be used. +/// affected). ProgramStateRef setErrnoForStdSuccess(ProgramStateRef State, CheckerContext &C); /// Set errno state for the common case when a standard function fails. @@ -100,23 +99,10 @@ ProgramStateRef setErrnoForStdFailure(ProgramStateRef State, CheckerContext &C, /// Set errno state for the common case when a standard function indicates /// failure only by \c errno. Sets \c ErrnoCheckState to \c MustBeChecked, and /// invalidates the errno region (clear of previous value). -/// At the state transition a note tag created by -/// \c getNoteTagForStdMustBeChecked can be used. /// \arg \c InvalE Expression that causes invalidation of \c errno. ProgramStateRef setErrnoStdMustBeChecked(ProgramStateRef State, CheckerContext &C, const Expr *InvalE); -/// Generate the note tag that can be applied at the state generated by -/// \c setErrnoForStdSuccess . -/// \arg \c Fn Name of the (standard) function that is modeled. -const NoteTag *getNoteTagForStdSuccess(CheckerContext &C, llvm::StringRef Fn); - -/// Generate the note tag that can be applied at the state generated by -/// \c setErrnoStdMustBeChecked . -/// \arg \c Fn Name of the (standard) function that is modeled. -const NoteTag *getNoteTagForStdMustBeChecked(CheckerContext &C, - llvm::StringRef Fn); - } // namespace errno_modeling } // namespace ento } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp index 54a41b8bd7843..2163689c6a059 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -533,13 +533,11 @@ class StdLibraryFunctionsChecker virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary, CheckerContext &C) const = 0; - /// Get a NoteTag about the changes made to 'errno' and the possible bug. - /// It may return \c nullptr (if no bug report from \c ErrnoChecker is - /// expected). - virtual const NoteTag *describe(CheckerContext &C, - StringRef FunctionName) const { - return nullptr; - } + /// Get a description about what happens with 'errno' here and how it causes + /// a later bug report created by ErrnoChecker. + /// Empty return value means that 'errno' related bug may not happen from + /// the current analyzed function. + virtual const std::string describe(CheckerContext &C) const { return ""; } virtual ~ErrnoConstraintBase() {} @@ -596,7 +594,8 @@ class StdLibraryFunctionsChecker }; /// Set errno constraint at success cases of standard functions. - /// Success case: 'errno' is not allowed to be used. + /// Success case: 'errno' is not allowed to be used because the value is + /// undefined after successful call. /// \c ErrnoChecker can emit bug report after such a function call if errno /// is used. class SuccessErrnoConstraint : public ErrnoConstraintBase { @@ -607,12 +606,15 @@ class StdLibraryFunctionsChecker return errno_modeling::setErrnoForStdSuccess(State, C); } - const NoteTag *describe(CheckerContext &C, - StringRef FunctionName) const override { - return errno_modeling::getNoteTagForStdSuccess(C, FunctionName); + const std::string describe(CheckerContext &C) const { + return "'errno' becomes undefined after the call"; } }; + /// Set errno constraint at functions that indicate failure only with 'errno'. + /// In this case 'errno' is required to be observed. + /// \c ErrnoChecker can emit bug report after such a function call if errno + /// is overwritten without a read before. class ErrnoMustBeCheckedConstraint : public ErrnoConstraintBase { public: ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, @@ -622,9 +624,8 @@ class StdLibraryFunctionsChecker Call.getOriginExpr()); } - const NoteTag *describe(CheckerContext &C, - StringRef FunctionName) const override { - return errno_modeling::getNoteTagForStdMustBeChecked(C, FunctionName); + const std::string describe(CheckerContext &C) const { + return "reading 'errno' is required to find out if the call has failed"; } }; @@ -1392,17 +1393,28 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, // Still add these note tags, the other checker should add only its // specialized note tags. These general note tags are handled always by // StdLibraryFunctionsChecker. + ExplodedNode *Pred = Node; - if (!Case.getNote().empty()) { - const SVal RV = Call.getReturnValue(); - // If there is a description for this execution branch (summary case), - // use it as a note tag. - std::string Note = - llvm::formatv(Case.getNote().str().c_str(), - cast(Call.getDecl())->getDeclName()); - if (Summary.getInvalidationKd() == EvalCallAsPure) { + DeclarationName FunctionName = + cast(Call.getDecl())->getDeclName(); + + std::string ErrnoNote = Case.getErrnoConstraint().describe(C); + std::string CaseNote; + if (Case.getNote().empty()) { + if (!ErrnoNote.empty()) + ErrnoNote = + llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote); + } else { + CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName); + } + const SVal RV = Call.getReturnValue(); + + if (Summary.getInvalidationKd() == EvalCallAsPure) { + // Do not expect that errno is interesting (the "pure" functions do not + // affect it). + if (!CaseNote.empty()) { const NoteTag *Tag = C.getNoteTag( - [Node, Note, RV](PathSensitiveBugReport &BR) -> std::string { + [Node, CaseNote, RV](PathSensitiveBugReport &BR) -> std::string { // Try to omit the note if we know in advance which branch is // taken (this means, only one branch exists). // This check is performed inside the lambda, after other @@ -1414,37 +1426,42 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, // the successors). This is why this check is only used in the // EvalCallAsPure case. if (BR.isInteresting(RV) && Node->succ_size() > 1) - return Note; + return CaseNote; return ""; }); Pred = C.addTransition(NewState, Pred, Tag); - } else { + } + } else { + if (!CaseNote.empty() || !ErrnoNote.empty()) { const NoteTag *Tag = - C.getNoteTag([Note, RV](PathSensitiveBugReport &BR) -> std::string { - if (BR.isInteresting(RV)) - return Note; + C.getNoteTag([CaseNote, ErrnoNote, + RV](PathSensitiveBugReport &BR) -> std::string { + // If 'errno' is interesting, show the user a note about the case + // (what happened at the function call) and about how 'errno' + // causes the problem. ErrnoChecker sets the errno (but not RV) to + // interesting. + // If only the return value is interesting, show only the case + // note. + std::optional ErrnoLoc = + errno_modeling::getErrnoLoc(BR.getErrorNode()->getState()); + bool ErrnoImportant = !ErrnoNote.empty() && ErrnoLoc && + BR.isInteresting(ErrnoLoc->getAsRegion()); + if (ErrnoImportant) { + BR.markNotInteresting(ErrnoLoc->getAsRegion()); + if (CaseNote.empty()) + return ErrnoNote; + return llvm::formatv("{0}; {1}", CaseNote, ErrnoNote); + } else { + if (BR.isInteresting(RV)) + return CaseNote; + } return ""; }); Pred = C.addTransition(NewState, Pred, Tag); } - - // Pred may be: - // - a nullpointer, if we reach an already existing node (theoretically); - // - a sink, when NewState is posteriorly overconstrained. - // In these situations we cannot add the errno note tag. - if (!Pred || Pred->isSink()) - continue; } - // If we can get a note tag for the errno change, add this additionally to - // the previous. This note is only about value of 'errno' and is displayed - // if 'errno' is interesting. - if (const auto *D = dyn_cast(Call.getDecl())) - if (const NoteTag *NT = - Case.getErrnoConstraint().describe(C, D->getNameAsString())) - Pred = C.addTransition(NewState, Pred, NT); - - // Add the transition if no note tag could be added. + // Add the transition if no note tag was added. if (Pred == Node && NewState != State) C.addTransition(NewState); } @@ -2038,7 +2055,8 @@ void StdLibraryFunctionsChecker::initFunctionSummaries( ErrnoMustNotBeChecked, GenericSuccessMsg) .Case({ArgumentCondition(1U, WithinRange, SingleValue(0)), ReturnValueCondition(WithinRange, SingleValue(0))}, - ErrnoMustNotBeChecked, GenericSuccessMsg) + ErrnoMustNotBeChecked, + "Assuming that argument 'size' to '{0}' is 0") .ArgConstraint(NotNullBuffer(ArgNo(0), ArgNo(1), ArgNo(2))) .ArgConstraint(NotNull(ArgNo(3))) .ArgConstraint(BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1), diff --git a/clang/test/Analysis/errno-stdlibraryfunctions-notes.c b/clang/test/Analysis/errno-stdlibraryfunctions-notes.c index c3fac58c46b37..41f54664cfb4f 100644 --- a/clang/test/Analysis/errno-stdlibraryfunctions-notes.c +++ b/clang/test/Analysis/errno-stdlibraryfunctions-notes.c @@ -15,7 +15,7 @@ void clang_analyzer_warnIfReached(); void test1() { access("path", 0); access("path", 0); - // expected-note@-1{{'errno' may be undefined after successful call to 'access'}} + // expected-note@-1{{Assuming that 'access' is successful; 'errno' becomes undefined after the call}} if (errno != 0) { // expected-warning@-1{{An undefined value may be read from 'errno'}} // expected-note@-2{{An undefined value may be read from 'errno'}} @@ -39,7 +39,7 @@ void test2() { void test3() { if (access("path", 0) != -1) { // Success path. - // expected-note@-2{{'errno' may be undefined after successful call to 'access'}} + // expected-note@-2{{Assuming that 'access' is successful; 'errno' becomes undefined after the call}} // expected-note@-3{{Taking true branch}} if (errno != 0) { // expected-warning@-1{{An undefined value may be read from 'errno'}} diff --git a/clang/test/Analysis/stream-errno-note.c b/clang/test/Analysis/stream-errno-note.c index 32d9d4fd9689d..d260eb2252f39 100644 --- a/clang/test/Analysis/stream-errno-note.c +++ b/clang/test/Analysis/stream-errno-note.c @@ -10,7 +10,7 @@ void check_fopen(void) { FILE *F = fopen("xxx", "r"); - // expected-note@-1{{'errno' may be undefined after successful call to 'fopen'}} + // expected-note@-1{{Assuming that 'fopen' is successful; 'errno' becomes undefined after the call}} // expected-note@+2{{'F' is non-null}} // expected-note@+1{{Taking false branch}} if (!F) @@ -22,7 +22,7 @@ void check_fopen(void) { void check_tmpfile(void) { FILE *F = tmpfile(); - // expected-note@-1{{'errno' may be undefined after successful call to 'tmpfile'}} + // expected-note@-1{{Assuming that 'tmpfile' is successful; 'errno' becomes undefined after the call}} // expected-note@+2{{'F' is non-null}} // expected-note@+1{{Taking false branch}} if (!F) @@ -39,7 +39,7 @@ void check_freopen(void) { if (!F) return; F = freopen("xxx", "w", F); - // expected-note@-1{{'errno' may be undefined after successful call to 'freopen'}} + // expected-note@-1{{Assuming that 'freopen' is successful; 'errno' becomes undefined after the call}} // expected-note@+2{{'F' is non-null}} // expected-note@+1{{Taking false branch}} if (!F) @@ -56,7 +56,7 @@ void check_fclose(void) { if (!F) return; (void)fclose(F); - // expected-note@-1{{'errno' may be undefined after successful call to 'fclose'}} + // expected-note@-1{{Assuming that 'fclose' 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'}} } @@ -69,7 +69,7 @@ void check_fread(void) { if (!F) return; (void)fread(Buf, 1, 10, F); - // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}} + // expected-note@-1{{Assuming that 'fread' 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); @@ -83,7 +83,7 @@ void check_fread_size0(void) { if (!F) return; fread(Buf, 0, 1, F); - // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}} + // expected-note@-1{{Assuming that argument 'size' to 'fread' is 0; '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'}} } @@ -96,7 +96,7 @@ void check_fread_nmemb0(void) { if (!F) return; fread(Buf, 1, 0, F); - // expected-note@-1{{'errno' may be undefined after successful call to 'fread'}} + // expected-note@-1{{Assuming that 'fread' 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'}} } @@ -109,7 +109,7 @@ void check_fwrite(void) { if (!F) return; int R = fwrite(Buf, 1, 10, F); - // expected-note@-1{{'errno' may be undefined after successful call to 'fwrite'}} + // expected-note@-1{{Assuming that 'fwrite' 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); @@ -122,7 +122,7 @@ void check_fseek(void) { if (!F) return; (void)fseek(F, 11, SEEK_SET); - // expected-note@-1{{'errno' may be undefined after successful call to 'fseek'}} + // expected-note@-1{{Assuming that 'fseek' 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); @@ -135,7 +135,7 @@ void check_rewind_errnocheck(void) { if (!F) return; errno = 0; - rewind(F); // expected-note{{'rewind' indicates failure only by setting 'errno'}} + rewind(F); // expected-note{{After calling 'rewind' reading 'errno' is required to find out if the call has failed}} fclose(F); // expected-warning{{Value of 'errno' was not checked and may be overwritten by function 'fclose' [alpha.unix.Errno]}} // expected-note@-1{{Value of 'errno' was not checked and may be overwritten by function 'fclose'}} } @@ -147,8 +147,27 @@ void check_fileno(void) { if (!F) return; fileno(F); - // expected-note@-1{{'errno' may be undefined after successful call to 'fileno'}} + // 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); } + +void check_fwrite_zeroarg(size_t Siz) { + char Buf[] = "0123456789"; + FILE *F = tmpfile(); + // expected-note@+2{{'F' is non-null}} + // expected-note@+1{{Taking false branch}} + if (!F) + return; + errno = 0; + int R = fwrite(Buf, Siz, 1, F); + // expected-note@-1{{Assuming that argument 'size' to 'fwrite' is 0; 'errno' becomes undefined after the call}} + // expected-note@+2{{'R' is <= 0}} + // expected-note@+1{{Taking true branch}} + if (R <= 0) { + 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); +}