[clang][analyzer] Add "pedantic" mode to StreamChecker. #87322
Merged
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
The checker may create failure branches for all stream write operations only if the new option "pedantic" is set to true. Result of the write operations is often not checked in typical code. If failure branches are created the checker will warn for unchecked write operations and generate a lot of "false positives" (these are valid warnings but the programmer does not care about this problem).
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesThe checker may create failure branches for all stream write operations only if the new option "pedantic" is set to true. Full diff: https://github.com/llvm/llvm-project/pull/87322.diff 9 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 5fe5c9286dabb7..570e0849e0124b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -604,6 +604,14 @@ def PthreadLockChecker : Checker<"PthreadLock">,
def StreamChecker : Checker<"Stream">,
HelpText<"Check stream handling functions">,
WeakDependencies<[NonNullParamChecker]>,
+ CheckerOptions<[
+ CmdLineOption<Boolean,
+ "Pedantic",
+ "If false, assume that stream write operations do never "
+ "fail.",
+ "false",
+ InAlpha>
+ ]>,
Documentation<HasDocumentation>;
def SimpleStreamChecker : Checker<"SimpleStream">,
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 069e3a633c1214..337420c3b25df3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -297,6 +297,9 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
/// If true, evaluate special testing stream functions.
bool TestMode = false;
+ /// If true, generate failure branches for cases that are often not checked.
+ bool PedanticMode = false;
+
private:
CallDescriptionMap<FnDescription> FnDescriptions = {
{{{"fopen"}, 2}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
@@ -945,6 +948,10 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
}
// Add transition for the failed state.
+ // At write, add failure case only if "pedantic mode" is on.
+ if (!IsFread && !PedanticMode)
+ return;
+
NonLoc RetVal = makeRetVal(C, E.CE).castAs<NonLoc>();
ProgramStateRef StateFailed =
State->BindExpr(E.CE, C.getLocationContext(), RetVal);
@@ -1059,6 +1066,9 @@ void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
// Add transition for the failed state. The resulting value of the file
// position indicator for the stream is indeterminate.
+ if (!PedanticMode)
+ return;
+
ProgramStateRef StateFailed = E.bindReturnValue(State, C, *EofVal);
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, ErrorFError, true));
@@ -1094,6 +1104,9 @@ void StreamChecker::evalFprintf(const FnDescription *Desc,
// Add transition for the failed state. The resulting value of the file
// position indicator for the stream is indeterminate.
+ if (!PedanticMode)
+ return;
+
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, ErrorFError, true));
C.addTransition(StateFailed, E.getFailureNoteTag(this, C));
@@ -1264,16 +1277,18 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
if (!E.Init(Desc, Call, C, State))
return;
- // Bifurcate the state into failed and non-failed.
- // Return zero on success, -1 on error.
+ // Add success state.
ProgramStateRef StateNotFailed = E.bindReturnValue(State, C, 0);
- ProgramStateRef StateFailed = E.bindReturnValue(State, C, -1);
-
// No failure: Reset the state to opened with no error.
StateNotFailed =
E.setStreamState(StateNotFailed, StreamState::getOpened(Desc));
C.addTransition(StateNotFailed);
+ // Add failure state.
+ if (!PedanticMode)
+ return;
+
+ ProgramStateRef StateFailed = E.bindReturnValue(State, C, -1);
// At error it is possible that fseek fails but sets none of the error flags.
// If fseek failed, assume that the file position becomes indeterminate in any
// case.
@@ -1316,6 +1331,10 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
StateNotFailed = E.setStreamState(
StateNotFailed, StreamState::getOpened(Desc, ErrorNone, false));
+ C.addTransition(StateNotFailed);
+
+ if (!PedanticMode)
+ return;
// At failure ferror could be set.
// The standards do not tell what happens with the file position at failure.
@@ -1324,7 +1343,6 @@ void StreamChecker::evalFsetpos(const FnDescription *Desc,
StateFailed = E.setStreamState(
StateFailed, StreamState::getOpened(Desc, ErrorNone | ErrorFError, true));
- C.addTransition(StateNotFailed);
C.addTransition(StateFailed, E.getFailureNoteTag(this, C));
}
@@ -1794,7 +1812,9 @@ ProgramStateRef StreamChecker::checkPointerEscape(
//===----------------------------------------------------------------------===//
void ento::registerStreamChecker(CheckerManager &Mgr) {
- Mgr.registerChecker<StreamChecker>();
+ auto *Checker = Mgr.registerChecker<StreamChecker>();
+ Checker->PedanticMode =
+ Mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker, "Pedantic");
}
bool ento::shouldRegisterStreamChecker(const CheckerManager &Mgr) {
diff --git a/clang/test/Analysis/stream-errno-note.c b/clang/test/Analysis/stream-errno-note.c
index 2411a2d9a00a72..fb12f0bace937f 100644
--- a/clang/test/Analysis/stream-errno-note.c
+++ b/clang/test/Analysis/stream-errno-note.c
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-checker=unix.Errno \
// RUN: -analyzer-checker=unix.StdCLibraryFunctions \
// RUN: -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true \
diff --git a/clang/test/Analysis/stream-errno.c b/clang/test/Analysis/stream-errno.c
index 5f0a58032fa263..08382eaf6abf9f 100644
--- a/clang/test/Analysis/stream-errno.c
+++ b/clang/test/Analysis/stream-errno.c
@@ -1,4 +1,5 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,unix.Errno,unix.StdCLibraryFunctions,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify %s
#include "Inputs/system-header-simulator.h"
diff --git a/clang/test/Analysis/stream-error.c b/clang/test/Analysis/stream-error.c
index 7f9116ff401445..2abf4b900a047f 100644
--- a/clang/test/Analysis/stream-error.c
+++ b/clang/test/Analysis/stream-error.c
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -verify %s \
// RUN: -analyzer-checker=core \
// RUN: -analyzer-checker=alpha.unix.Stream \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-checker=debug.StreamTester \
// RUN: -analyzer-checker=debug.ExprInspection
diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c
index 54ea699f46674e..03a8ff4e468f66 100644
--- a/clang/test/Analysis/stream-note.c
+++ b/clang/test/Analysis/stream-note.c
@@ -1,6 +1,8 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream -analyzer-output text \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -verify %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,unix.StdCLibraryFunctions -analyzer-output text \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify=expected,stdargs %s
#include "Inputs/system-header-simulator.h"
diff --git a/clang/test/Analysis/stream-pedantic.c b/clang/test/Analysis/stream-pedantic.c
new file mode 100644
index 00000000000000..5cdf70f1630eb2
--- /dev/null
+++ b/clang/test/Analysis/stream-pedantic.c
@@ -0,0 +1,71 @@
+// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=false -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+void check_fwrite(void) {
+ char *Buf = "123456789";
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ size_t Ret = fwrite(Buf, 1, 10, Fp);
+ clang_analyzer_eval(Ret == 0); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
+
+void check_fputc(void) {
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ int Ret = fputc('A', Fp);
+ clang_analyzer_eval(Ret == EOF); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
+
+void check_fputs(void) {
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ int Ret = fputs("ABC", Fp);
+ clang_analyzer_eval(Ret == EOF); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
+
+void check_fprintf(void) {
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ int Ret = fprintf(Fp, "ABC");
+ clang_analyzer_eval(Ret < 0); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
+
+void check_fseek(void) {
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ int Ret = fseek(Fp, 0, 0);
+ clang_analyzer_eval(Ret == -1); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
+
+void check_fseeko(void) {
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ int Ret = fseeko(Fp, 0, 0);
+ clang_analyzer_eval(Ret == -1); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
+
+void check_fsetpos(void) {
+ FILE *Fp = tmpfile();
+ if (!Fp)
+ return;
+ fpos_t Pos;
+ int Ret = fsetpos(Fp, &Pos);
+ clang_analyzer_eval(Ret); // expected-warning {{FALSE}}
+ fclose(Fp);
+}
diff --git a/clang/test/Analysis/stream-stdlibraryfunctionargs.c b/clang/test/Analysis/stream-stdlibraryfunctionargs.c
index 0053510163efc0..2ea6a8c472c613 100644
--- a/clang/test/Analysis/stream-stdlibraryfunctionargs.c
+++ b/clang/test/Analysis/stream-stdlibraryfunctionargs.c
@@ -1,10 +1,13 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,unix.StdCLibraryFunctions,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stream,any %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stream,any %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.StdCLibraryFunctions,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true \
// RUN: -analyzer-config unix.StdCLibraryFunctions:ModelPOSIX=true -verify=stdfunc,any %s
#include "Inputs/system-header-simulator.h"
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index ba5e66a4102e3c..93ed555c89ebd1 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -1,7 +1,11 @@
-// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
-// RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
-// RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
-// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true -verify %s
+// RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true -verify %s
+// RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true -verify %s
+// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,alpha.unix.Stream,debug.ExprInspection \
+// RUN: -analyzer-config alpha.unix.Stream:Pedantic=true -verify %s
#include "Inputs/system-header-simulator.h"
#include "Inputs/system-header-simulator-for-malloc.h"
|
NagyDonat
approved these changes
Apr 2, 2024
There was a problem hiding this comment.
Good change, I have a few minor suggestions that tweak comment placement.
I felt that it's confusing to have an "Add a transition for" comment directly followed by a seemingly unrelated if (!PedanticMode) block -- my suggestions swap the comment and the if block, but you could also e.g. prefix the comment with "In pedantic mode, also..." to clarify the logical connection between the if and the rest of the comment.
As an unrelated note, please explain this checker option to the documentation of the checker (in addition to the Checkers.td change).
Comment on lines
1067
to
1071
| // Add transition for the failed state. The resulting value of the file | ||
| // position indicator for the stream is indeterminate. | ||
| if (!PedanticMode) | ||
| return; | ||
|
|
There was a problem hiding this comment.
Suggested change
| // Add transition for the failed state. The resulting value of the file | |
| // position indicator for the stream is indeterminate. | |
| if (!PedanticMode) | |
| return; | |
| if (!PedanticMode) | |
| return; | |
| // Add transition for the failed state. The resulting value of the file | |
| // position indicator for the stream is indeterminate. |
Comment on lines
1105
to
1109
| // Add transition for the failed state. The resulting value of the file | ||
| // position indicator for the stream is indeterminate. | ||
| if (!PedanticMode) | ||
| return; | ||
|
|
There was a problem hiding this comment.
Suggested change
| // Add transition for the failed state. The resulting value of the file | |
| // position indicator for the stream is indeterminate. | |
| if (!PedanticMode) | |
| return; | |
| if (!PedanticMode) | |
| return; | |
| // Add transition for the failed state. The resulting value of the file | |
| // position indicator for the stream is indeterminate. |
| // No failure: Reset the state to opened with no error. | ||
| StateNotFailed = | ||
| E.setStreamState(StateNotFailed, StreamState::getOpened(Desc)); | ||
| C.addTransition(StateNotFailed); | ||
|
|
||
| // Add failure state. |
There was a problem hiding this comment.
Suggested change
| // Add failure state. | |
| // In pedantic mode, also add a failure state. |
steakhal
reviewed
Apr 2, 2024
|
Personally I like the habit that these options are consistently named "Pedantic" because it highlights that they all provide "true, but not helpful" results. When the users see that an option is named "Pedantic", many of them will be able to immediately conclude that they are not interested; while if the option was named "AssumeSuccessfulWrites", they would need to understand its exact meaning (so they would probably read the full doc, not just the name) and its relevance in their project to make a decision. I agree that "AssumeSuccessfulWrites" is more informative in a certain sense, but it still doesn't convey enough information for a full understanding; and "Pedantic" provides a different kind of information that (IMO) is more practical overall. (But I'm also fine with renaming the option if you feel that that'd be better.) |
I do not like totally the name "Pedantic", it could be "AssumeOftenUncheckedOperationsMayFail". I am not sure if this behavior is needed only on write operations, the intent was to remove failure branches from all operations that are often unchecked. |
|
I feel that "AssumeOftenUncheckedOperationsMayFail" does not provide more information than "Pedantic" (≈ report issues that are often left unchecked), while it is significantly longer, so my preferences are Pedantic > AssumeSuccessfulWrites > AssumeOftenUncheckedOperationsMayFail. |
The docs within
I think we can disagree. One remark, inside the |
* Add documentation of new option * Update failing tests * Improve test by adding checks for non-pedantic mode
NagyDonat
approved these changes
Apr 4, 2024
clang/docs/analyzer/checkers.rst
Outdated
Comment on lines
3141
to
3147
| The checker assumes by default that any of the stream operations, except the | ||
| write operations (like ``fwrite``, ``fprintf``) can fail. The option | ||
| ``Pedantic`` controls if the write operations are assumed to be successful or | ||
| not. If set to ``true``, the write operations (including change of file | ||
| position) can fail too (default is ``false``). This option was added to prevent | ||
| many checker warnings if the code contains many write operations without error | ||
| check. |
There was a problem hiding this comment.
Suggested change
| The checker assumes by default that any of the stream operations, except the | |
| write operations (like ``fwrite``, ``fprintf``) can fail. The option | |
| ``Pedantic`` controls if the write operations are assumed to be successful or | |
| not. If set to ``true``, the write operations (including change of file | |
| position) can fail too (default is ``false``). This option was added to prevent | |
| many checker warnings if the code contains many write operations without error | |
| check. | |
| When this checker models a stream operation, it usually splits the analysis into | |
| two cases: one where the stream operation succeeds, and one where it fails. | |
| However, in the case of write operations (like ``fwrite``, ``fprintf` | |
| and even `fsetpos`) this behavior could produce a large amount of unwanted | |
| reports on projects that don't have error checks around the write operations -- | |
| so by default the checker assumes that write operations always succeed. | |
| This behavior can be controlled by the ``Pedantic`` flag: with | |
| ``-analyzer-config alpha.unix.Stream:Pedantic=true`` the checker will model the | |
| cases where a write operation fails and report situations where this leads to | |
| erroneous behavior. (The default is ``Pedantic=false``, where write operations | |
| are assumed to succeed.) |
I felt that this paragraph was a bit difficult to understand, so I rewrote it in a somewhat more verbose and hopefully clearer way. Feel free to tweak this if you feel that there could be further improvements.
Add change of documentation.
NagyDonat
approved these changes
Apr 8, 2024
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.
The checker may create failure branches for all stream write operations only if the new option "pedantic" is set to true.
Result of the write operations is often not checked in typical code. If failure branches are created the checker will warn for unchecked write operations and generate a lot of "false positives" (these are valid warnings but the programmer does not care about this problem).