[PATCH] D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash
martong abandoned this revision. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:694 // execution continues on a code whose behaviour is undefined. assert(SuccessSt); NewState = SuccessSt; martong wrote: > This is where we crashed before this fix. `assert(SuccessSt);` should not ever fail. Seems like the logic is not flawed in `negate` rather there is an issue in the underlying `RangeConstraintManager`: the analyzer goes on with an unfeasible path. See the post-commit comments here: https://reviews.llvm.org/D82445 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87785/new/ https://reviews.llvm.org/D87785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash
martong added a comment. Alright, I refactored the change a bit. A Constraint::assume works similarly to an engine DualAssume. Plus I added `isApplicable` to check if it is meaningful to call the assume at all. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87785/new/ https://reviews.llvm.org/D87785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash
martong updated this revision to Diff 292550.
martong added a comment.
- apply -> assume, Add isApplicable
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87785/new/
https://reviews.llvm.org/D87785
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions.c
Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -126,6 +126,25 @@
(void)fread(ptr, sz, nmem, fp); // expected-warning {{1st function call argument is an uninitialized value}}
}
+static void test_fread_buffer_size_constraint_no_crash(FILE *f, size_t n) {
+ size_t rlen; /* how much to read */
+ size_t nr; /* number of chars actually read */
+ char b[8192];
+ rlen = 8192;
+ do {
+char *p = b;
+if (rlen > n)
+ rlen = n; /* cannot read more than asked */
+nr = fread(p, sizeof(char), rlen, f);
+n -= nr; /* still have to read `n' chars */
+ } while (n > 0 && nr == rlen); /* until end of count or eof */
+}
+// Test that we do not crash here.
+void test_fread_buffer_size_constraint_no_crash_caller(FILE *f) {
+ /* read MAX_SIZE_T chars */
+ test_fread_buffer_size_constraint_no_crash(f, ~((size_t)0));
+}
+
ssize_t getline(char **, size_t *, FILE *);
void test_getline(FILE *fp) {
char *line = 0;
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -104,14 +104,18 @@
public:
ValueConstraint(ArgNo ArgN) : ArgN(ArgN) {}
virtual ~ValueConstraint() {}
-/// Apply the effects of the constraint on the given program state. If null
-/// is returned then the constraint is not feasible.
-virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
- const Summary ,
- CheckerContext ) const = 0;
-virtual ValueConstraintPtr negate() const {
- llvm_unreachable("Not implemented");
-};
+
+/// Returns true if we can do any assumption on the constraint, i.e. if it
+/// is meaningful to call an Engine `assume` function.
+virtual bool isApplicable(ProgramStateRef State, const CallEvent ,
+ const Summary ,
+ CheckerContext ) const = 0;
+
+/// Returns the program states for the cases when the constraint is
+/// satisfiable and non satisfiable.
+virtual std::pair
+assume(ProgramStateRef State, const CallEvent , const Summary ,
+ CheckerContext ) const = 0;
// Check whether the constraint is malformed or not. It is malformed if the
// specified argument has a mismatch with the given FunctionDecl (e.g. the
@@ -169,29 +173,25 @@
const Summary ) const;
public:
-ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
- const Summary ,
- CheckerContext ) const override {
- switch (Kind) {
- case OutOfRange:
-return applyAsOutOfRange(State, Call, Summary);
- case WithinRange:
-return applyAsWithinRange(State, Call, Summary);
- }
- llvm_unreachable("Unknown range kind!");
+bool isApplicable(ProgramStateRef State, const CallEvent ,
+ const Summary ,
+ CheckerContext ) const override {
+ SVal V = getArgSVal(Call, getArgNo());
+ return V.getAs().hasValue();
}
-ValueConstraintPtr negate() const override {
- RangeConstraint Tmp(*this);
+std::pair
+assume(ProgramStateRef State, const CallEvent , const Summary ,
+ CheckerContext ) const override {
switch (Kind) {
case OutOfRange:
-Tmp.Kind = WithinRange;
-break;
+return {applyAsOutOfRange(State, Call, Summary),
+applyAsWithinRange(State, Call, Summary)};
case WithinRange:
-Tmp.Kind = OutOfRange;
-break;
+return {applyAsWithinRange(State, Call, Summary),
+applyAsOutOfRange(State, Call, Summary)};
}
- return std::make_shared(Tmp);
+ llvm_unreachable("Unknown range kind!");
}
bool checkSpecificValidity(const FunctionDecl *FD) const override {
@@ -214,36 +214,37 @@
: ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
ArgNo getOtherArgNo() const { return OtherArgN; }
BinaryOperator::Opcode getOpcode() const { return Opcode; }
-ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
- const Summary ,
- CheckerContext )
[PATCH] D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash
steakhal added inline comments.
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:219-226
if (V.isUndef())
-return State;
+return {State, State};
DefinedOrUnknownSVal L = V.castAs();
if (!L.getAs())
-return State;
-
- return State->assume(L, CannotBeNull);
-}
+return {State, State};
I suggest the same //simpler// version for the similar code segments as well.
By the same token, why do you return `{State, State}`?
Shouldn't you return `{State, nullptr}` there?
In general, one would not expect the **same** State being returned, IMO it's
advised to avoid doing that.
Same applies for the other cases.
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:299-300
SvalBuilder.getContext().BoolTy);
if (auto F = Feasible.getAs())
-return State->assume(*F, true);
+return State->assume(*F);
Why don't you `castAs`? That also has the corresponding assert inside.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87785/new/
https://reviews.llvm.org/D87785
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash
martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:694 // execution continues on a code whose behaviour is undefined. assert(SuccessSt); NewState = SuccessSt; This is where we crashed before this fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87785/new/ https://reviews.llvm.org/D87785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash
martong created this revision.
martong added reviewers: steakhal, balazske, Szelethus, NoQ.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411,
dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet,
baloghadamsoftware, xazax.hun, whisperity.
Herald added a project: clang.
martong requested review of this revision.
It could happen that both the satisfied and unsatisfied constraints gave
a NULL state. This happened probably because the negate() functionality
was not perfect. The solution is to return both states from the `assume`
calls where it makes sense.
This way the code becomes cleaner, there is no need anymore for the
controversial `negate()`.
Note that this causes a regression in our CTU builds.
http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/1720/
The error itself is not CTU related, however, the coverage and thus the path is
different in that mode.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D87785
Files:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
clang/test/Analysis/std-c-library-functions.c
Index: clang/test/Analysis/std-c-library-functions.c
===
--- clang/test/Analysis/std-c-library-functions.c
+++ clang/test/Analysis/std-c-library-functions.c
@@ -126,6 +126,25 @@
(void)fread(ptr, sz, nmem, fp); // expected-warning {{1st function call argument is an uninitialized value}}
}
+static void test_fread_buffer_size_constraint_no_crash(FILE *f, size_t n) {
+ size_t rlen; /* how much to read */
+ size_t nr; /* number of chars actually read */
+ char b[8192];
+ rlen = 8192;
+ do {
+char *p = b;
+if (rlen > n)
+ rlen = n; /* cannot read more than asked */
+nr = fread(p, sizeof(char), rlen, f);
+n -= nr; /* still have to read `n' chars */
+ } while (n > 0 && nr == rlen); /* until end of count or eof */
+}
+// Test that we do not crash here.
+void test_fread_buffer_size_constraint_no_crash_caller(FILE *f) {
+ /* read MAX_SIZE_T chars */
+ test_fread_buffer_size_constraint_no_crash(f, ~((size_t)0));
+}
+
ssize_t getline(char **, size_t *, FILE *);
void test_getline(FILE *fp) {
char *line = 0;
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -104,14 +104,13 @@
public:
ValueConstraint(ArgNo ArgN) : ArgN(ArgN) {}
virtual ~ValueConstraint() {}
-/// Apply the effects of the constraint on the given program state. If null
-/// is returned then the constraint is not feasible.
-virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
- const Summary ,
- CheckerContext ) const = 0;
-virtual ValueConstraintPtr negate() const {
- llvm_unreachable("Not implemented");
-};
+
+/// Apply the effects of the constraint on the given program state. Returns
+/// the program states for the cases when the constraint is satisfiable and
+/// non satisfiable.
+virtual std::pair
+apply(ProgramStateRef State, const CallEvent , const Summary ,
+ CheckerContext ) const = 0;
// Check whether the constraint is malformed or not. It is malformed if the
// specified argument has a mismatch with the given FunctionDecl (e.g. the
@@ -169,31 +168,20 @@
const Summary ) const;
public:
-ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
- const Summary ,
- CheckerContext ) const override {
+std::pair
+apply(ProgramStateRef State, const CallEvent , const Summary ,
+ CheckerContext ) const override {
switch (Kind) {
case OutOfRange:
-return applyAsOutOfRange(State, Call, Summary);
+return {applyAsOutOfRange(State, Call, Summary),
+applyAsWithinRange(State, Call, Summary)};
case WithinRange:
-return applyAsWithinRange(State, Call, Summary);
+return {applyAsWithinRange(State, Call, Summary),
+applyAsOutOfRange(State, Call, Summary)};
}
llvm_unreachable("Unknown range kind!");
}
-ValueConstraintPtr negate() const override {
- RangeConstraint Tmp(*this);
- switch (Kind) {
- case OutOfRange:
-Tmp.Kind = WithinRange;
-break;
- case WithinRange:
-Tmp.Kind = OutOfRange;
-break;
- }
- return std::make_shared(Tmp);
-}
-
bool checkSpecificValidity(const FunctionDecl *FD) const override {
const bool ValidArg =
getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
@@ -214,36 +202,28 @@
:
