Skip to content

[clang][analyzer] Improve 'errno' handling in StdLibraryFunctionsChecker. #71392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 16, 2023
Merged

[clang][analyzer] Improve 'errno' handling in StdLibraryFunctionsChecker. #71392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e92bf72
[clang][analyzer] Improve 'errno' handling in StdLibraryFunctionsChec…
balazske Nov 2, 2023
76a0e1e
remove unrelated test
balazske Nov 8, 2023
44bdb32
fixed review issues
balazske Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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);
}