[clang][analyzer] Fix argument invalidations in StreamChecker. #79470
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesSpecific arguments passed to stream handling functions are changed by the function, this means these should be invalidated ("escaped") by the analyzer. This change adds the argument invalidation (in specific cases) to the checker. Full diff: https://github.com/llvm/llvm-project/pull/79470.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..166bd981a003af2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
#include <functional>
#include <optional>
@@ -544,6 +545,21 @@ const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
return nullptr;
}
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+ const SmallVector<unsigned int> &EscapingArgs) {
+ const auto *CE = Call.getOriginExpr();
+
+ SmallVector<SVal> EscapingVals;
+ EscapingVals.reserve(EscapingArgs.size());
+ for (auto EscArgIdx : EscapingArgs)
+ EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+ State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+ C.getLocationContext(),
+ /*CausesPointerEscape=*/false);
+ return State;
+}
+
//===----------------------------------------------------------------------===//
// Methods of StreamChecker.
//===----------------------------------------------------------------------===//
@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
return;
}
+ // At read, invalidate the buffer in any case of error or success,
+ // except if EOF was already present.
+ if (IsFread && (OldSS->ErrorState != ErrorFEof))
+ State = escapeArgs(State, C, Call, {0});
+
// Generate a transition for the success state.
// If we know the state to be FEOF at fread, do not add a success state.
if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
@@ -824,6 +845,10 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
// `fgets` returns the read buffer address on success, otherwise returns NULL.
if (OldSS->ErrorState != ErrorFEof) {
+ // If there was already EOF, assume that read buffer is not changed.
+ // Otherwise it may change at success or failure.
+
+ State = escapeArgs(State, C, Call, {0});
if (SingleChar) {
// Generate a transition for the success state of `fgetc`.
NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
return;
StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
+ SmallVector<unsigned int> EscArgs;
+ for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+ EscArgs.push_back(EscArg);
+ StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
C.addTransition(StateNotFailed);
}
@@ -1118,6 +1148,10 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
// Add transition for the successful state.
if (OldSS->ErrorState != ErrorFEof) {
+ // Escape buffer and size (may change by the call).
+ // May happen even at error (partial read?).
+ State = escapeArgs(State, C, Call, {0, 1});
+
NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
ProgramStateRef StateNotFailed =
State->BindExpr(CE, C.getLocationContext(), RetVal);
@@ -1236,6 +1270,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
ProgramStateRef StateNotFailed, StateFailed;
std::tie(StateFailed, StateNotFailed) =
C.getConstraintManager().assumeDual(State, RetVal);
+ StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
// This function does not affect the stream state.
// Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
new file mode 100644
index 000000000000000..c5b7874f0ee8f6f
--- /dev/null
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+
+ char Buf[3] = {10, 10, 10};
+ fread(Buf, 1, 3, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
+ clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+ if (feof(F)) {
+ char Buf1[3] = {10, 10, 10};
+ fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
+ clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+ clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+ }
+
+ fclose(F);
+}
+
+void test_fwrite(void) {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+
+ char Buf[3] = {10, 10, 10};
+ fwrite(Buf, 1, 3, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
+ clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}
+
+ fclose(F);
+}
+
+void test_fgets() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ char Buf[3] = {10, 10, 10};
+ fgets(Buf, 3, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
+ clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+ if (feof(F)) {
+ char Buf1[3] = {10, 10, 10};
+ fgets(Buf1, 3, F); // expected-warning {{is in EOF state}}
+ clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+ clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+ }
+
+ fclose(F);
+}
+
+void test_fputs() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ char *Buf = "aaa";
+ fputs(Buf, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{97 S32b}}
+ clang_analyzer_dump(Buf[2]); // expected-warning {{97 S32b}}
+ clang_analyzer_dump(Buf[3]); // expected-warning {{0 S32b}}
+
+ fclose(F);
+}
+
+void test_fscanf() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ int a = 1;
+ unsigned b;
+ int Ret = fscanf(F, "%d %u", &a, &b);
+ if (Ret >= 0) {
+ // FIXME: return value
+ clang_analyzer_dump(a); // expected-warning {{conj_$}}
+ clang_analyzer_dump(b); // expected-warning {{conj_$}}
+ } else {
+ clang_analyzer_dump(a); // expected-warning {{1 S32b}}
+ clang_analyzer_dump(b); // expected-warning {{uninitialized value}}
+ }
+ fclose(F);
+}
+
+void test_getdelim(char *P, size_t Sz) {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ char *P1 = P;
+ size_t Sz1 = Sz;
+ ssize_t Ret = getdelim(&P, &Sz, '\t', F);
+ clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+ clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+
+ fclose(F);
+}
+
+void test_fgetpos() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ fpos_t Pos = 1;
+ int Ret = fgetpos(F, &Pos);
+ if (Ret == 0) {
+ clang_analyzer_dump(Pos); // expected-warning {{conj_$}}
+ } else {
+ clang_analyzer_dump(Pos); // expected-warning {{1 S32b}}
+ }
+
+ fclose(F);
+}
|
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesSpecific arguments passed to stream handling functions are changed by the function, this means these should be invalidated ("escaped") by the analyzer. This change adds the argument invalidation (in specific cases) to the checker. Full diff: https://github.com/llvm/llvm-project/pull/79470.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..166bd981a003af2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -21,6 +21,7 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "llvm/ADT/Sequence.h"
#include <functional>
#include <optional>
@@ -544,6 +545,21 @@ const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
return nullptr;
}
+static ProgramStateRef
+escapeArgs(ProgramStateRef State, CheckerContext &C, const CallEvent &Call,
+ const SmallVector<unsigned int> &EscapingArgs) {
+ const auto *CE = Call.getOriginExpr();
+
+ SmallVector<SVal> EscapingVals;
+ EscapingVals.reserve(EscapingArgs.size());
+ for (auto EscArgIdx : EscapingArgs)
+ EscapingVals.push_back(Call.getArgSVal(EscArgIdx));
+ State = State->invalidateRegions(EscapingVals, CE, C.blockCount(),
+ C.getLocationContext(),
+ /*CausesPointerEscape=*/false);
+ return State;
+}
+
//===----------------------------------------------------------------------===//
// Methods of StreamChecker.
//===----------------------------------------------------------------------===//
@@ -763,6 +779,11 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
return;
}
+ // At read, invalidate the buffer in any case of error or success,
+ // except if EOF was already present.
+ if (IsFread && (OldSS->ErrorState != ErrorFEof))
+ State = escapeArgs(State, C, Call, {0});
+
// Generate a transition for the success state.
// If we know the state to be FEOF at fread, do not add a success state.
if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
@@ -824,6 +845,10 @@ void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
// `fgets` returns the read buffer address on success, otherwise returns NULL.
if (OldSS->ErrorState != ErrorFEof) {
+ // If there was already EOF, assume that read buffer is not changed.
+ // Otherwise it may change at success or failure.
+
+ State = escapeArgs(State, C, Call, {0});
if (SingleChar) {
// Generate a transition for the success state of `fgetc`.
NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
@@ -1032,6 +1057,11 @@ void StreamChecker::evalFscanf(const FnDescription *Desc, const CallEvent &Call,
return;
StateNotFailed = StateNotFailed->assume(*RetGeZero, true);
+ SmallVector<unsigned int> EscArgs;
+ for (auto EscArg : llvm::seq(2u, Call.getNumArgs()))
+ EscArgs.push_back(EscArg);
+ StateNotFailed = escapeArgs(StateNotFailed, C, Call, EscArgs);
+
C.addTransition(StateNotFailed);
}
@@ -1118,6 +1148,10 @@ void StreamChecker::evalGetdelim(const FnDescription *Desc,
// Add transition for the successful state.
if (OldSS->ErrorState != ErrorFEof) {
+ // Escape buffer and size (may change by the call).
+ // May happen even at error (partial read?).
+ State = escapeArgs(State, C, Call, {0, 1});
+
NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
ProgramStateRef StateNotFailed =
State->BindExpr(CE, C.getLocationContext(), RetVal);
@@ -1236,6 +1270,7 @@ void StreamChecker::evalFgetpos(const FnDescription *Desc,
ProgramStateRef StateNotFailed, StateFailed;
std::tie(StateFailed, StateNotFailed) =
C.getConstraintManager().assumeDual(State, RetVal);
+ StateNotFailed = escapeArgs(StateNotFailed, C, Call, {1});
// This function does not affect the stream state.
// Still we add success and failure state with the appropriate return value.
diff --git a/clang/test/Analysis/stream-invalidate.c b/clang/test/Analysis/stream-invalidate.c
new file mode 100644
index 000000000000000..c5b7874f0ee8f6f
--- /dev/null
+++ b/clang/test/Analysis/stream-invalidate.c
@@ -0,0 +1,133 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-checker=debug.StreamTester \
+// RUN: -analyzer-checker=debug.ExprInspection
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int);
+void clang_analyzer_warnIfReached(void);
+void StreamTesterChecker_make_feof_stream(FILE *);
+void StreamTesterChecker_make_ferror_stream(FILE *);
+
+void test_fread(void) {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+
+ char Buf[3] = {10, 10, 10};
+ fread(Buf, 1, 3, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
+ clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+ if (feof(F)) {
+ char Buf1[3] = {10, 10, 10};
+ fread(Buf1, 1, 3, F); // expected-warning {{is in EOF state}}
+ clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+ clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+ }
+
+ fclose(F);
+}
+
+void test_fwrite(void) {
+ FILE *F = fopen("file", "r+");
+ if (!F)
+ return;
+
+ char Buf[3] = {10, 10, 10};
+ fwrite(Buf, 1, 3, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{10 S32b}}
+ clang_analyzer_dump(Buf[2]); // expected-warning {{10 S32b}}
+
+ fclose(F);
+}
+
+void test_fgets() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ char Buf[3] = {10, 10, 10};
+ fgets(Buf, 3, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{conj_$}} Should not preserve the previous value, thus should not be 10.
+ clang_analyzer_dump(Buf[2]); // expected-warning {{conj_$}}
+ if (feof(F)) {
+ char Buf1[3] = {10, 10, 10};
+ fgets(Buf1, 3, F); // expected-warning {{is in EOF state}}
+ clang_analyzer_dump(Buf1[0]); // expected-warning {{10 S32b}}
+ clang_analyzer_dump(Buf1[2]); // expected-warning {{10 S32b}}
+ }
+
+ fclose(F);
+}
+
+void test_fputs() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ char *Buf = "aaa";
+ fputs(Buf, F);
+ // this check applies to succes and failure
+ clang_analyzer_dump(Buf[0]); // expected-warning {{97 S32b}}
+ clang_analyzer_dump(Buf[2]); // expected-warning {{97 S32b}}
+ clang_analyzer_dump(Buf[3]); // expected-warning {{0 S32b}}
+
+ fclose(F);
+}
+
+void test_fscanf() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ int a = 1;
+ unsigned b;
+ int Ret = fscanf(F, "%d %u", &a, &b);
+ if (Ret >= 0) {
+ // FIXME: return value
+ clang_analyzer_dump(a); // expected-warning {{conj_$}}
+ clang_analyzer_dump(b); // expected-warning {{conj_$}}
+ } else {
+ clang_analyzer_dump(a); // expected-warning {{1 S32b}}
+ clang_analyzer_dump(b); // expected-warning {{uninitialized value}}
+ }
+ fclose(F);
+}
+
+void test_getdelim(char *P, size_t Sz) {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ char *P1 = P;
+ size_t Sz1 = Sz;
+ ssize_t Ret = getdelim(&P, &Sz, '\t', F);
+ clang_analyzer_eval(P == P1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+ clang_analyzer_eval(Sz == Sz1); // expected-warning {{FALSE}} \
+ // expected-warning {{TRUE}}
+
+ fclose(F);
+}
+
+void test_fgetpos() {
+ FILE *F = tmpfile();
+ if (!F)
+ return;
+
+ fpos_t Pos = 1;
+ int Ret = fgetpos(F, &Pos);
+ if (Ret == 0) {
+ clang_analyzer_dump(Pos); // expected-warning {{conj_$}}
+ } else {
+ clang_analyzer_dump(Pos); // expected-warning {{1 S32b}}
+ }
+
+ fclose(F);
+}
|
steakhal
reviewed
Jan 25, 2024
steakhal
approved these changes
Feb 21, 2024
There was a problem hiding this comment.
Thanks for resolving my comments.
FYI if I forget about a PR (that I promise to come back on the next day) - feel free to ping it or explicitly push the "request review" button.
Wait for my collage to also have a look, as I believe he might be in context to review this change. @alejandro-alvarez-sonarsource
Specific arguments passed to stream handling functions are changed by the function,
this means these should be invalidated ("escaped") by the analyzer.
This change adds the argument invalidation (in specific cases) to the checker.
balazske
force-pushed
the
streamchecker_invalidate
branch
from
8834232 to
70eeae8
Compare
|
I have rebased the branch to latest version of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specific arguments passed to stream handling functions are changed by the function, this means these should be invalidated ("escaped") by the analyzer. This change adds the argument invalidation (in specific cases) to the checker.