Skip to content

Commit

Permalink
[clang][analyzer] Improve 'errno' handling in StdLibraryFunctionsChec…
Browse files Browse the repository at this point in the history 
…ker. (#71392)

The checker now displays one combined note tag for errno-related and
"case"-related notes. Previous functions in the errno-modeling part that
were used for construction of note tags are removed. The note tag added
by StdLibraryFunctionsChecker contains the code to display the note tag
for 'errno' (this was done previously by these removed functions).
  • Loading branch information
@balazske
balazske authored Nov 16, 2023
1 parent ebbb9cd commit 699e101
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 85 deletions.
12 changes: 0 additions & 12 deletions clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 1 addition & 15 deletions clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
108 changes: 63 additions & 45 deletions clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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";
}
};

Expand Down Expand Up @@ -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<NamedDecl>(Call.getDecl())->getDeclName());
if (Summary.getInvalidationKd() == EvalCallAsPure) {
DeclarationName FunctionName =
cast<NamedDecl>(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
Expand All @@ -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<Loc> 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<FunctionDecl>(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);
}
Expand Down Expand Up @@ -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),
Expand Down
4 changes: 2 additions & 2 deletions clang/test/Analysis/errno-stdlibraryfunctions-notes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'}}
Expand All @@ -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'}}
Expand Down
41 changes: 30 additions & 11 deletions clang/test/Analysis/stream-errno-note.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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'}}
}
Expand All @@ -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);
Expand All @@ -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'}}
}
Expand All @@ -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'}}
}
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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'}}
}
Expand All @@ -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);
}

0 comments on commit 699e101

Please sign in to comment.