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 &Call,
- const Summary &Summary,
- CheckerContext &C) 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<ProgramStateRef, ProgramStateRef>
+ apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
+ CheckerContext &C) 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 &Summary) const;
public:
- ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
- const Summary &Summary,
- CheckerContext &C) const override {
+ std::pair<ProgramStateRef, ProgramStateRef>
+ apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
+ CheckerContext &C) 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<RangeConstraint>(Tmp);
- }
-
bool checkSpecificValidity(const FunctionDecl *FD) const override {
const bool ValidArg =
getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
@@ -214,36 +202,28 @@
: ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
ArgNo getOtherArgNo() const { return OtherArgN; }
BinaryOperator::Opcode getOpcode() const { return Opcode; }
- ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
- const Summary &Summary,
- CheckerContext &C) const override;
+ std::pair<ProgramStateRef, ProgramStateRef>
+ apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
+ CheckerContext &C) const override;
};
class NotNullConstraint : public ValueConstraint {
using ValueConstraint::ValueConstraint;
- // This variable has a role when we negate the constraint.
- bool CannotBeNull = true;
public:
StringRef getName() const override { return "NonNull"; }
- ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
- const Summary &Summary,
- CheckerContext &C) const override {
+ std::pair<ProgramStateRef, ProgramStateRef>
+ apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
+ CheckerContext &C) const override {
SVal V = getArgSVal(Call, getArgNo());
if (V.isUndef())
- return State;
+ return {State, State};
DefinedOrUnknownSVal L = V.castAs<DefinedOrUnknownSVal>();
if (!L.getAs<Loc>())
- return State;
-
- return State->assume(L, CannotBeNull);
- }
+ return {State, State};
- ValueConstraintPtr negate() const override {
- NotNullConstraint Tmp(*this);
- Tmp.CannotBeNull = !this->CannotBeNull;
- return std::make_shared<NotNullConstraint>(Tmp);
+ return State->assume(L);
}
bool checkSpecificValidity(const FunctionDecl *FD) const override {
@@ -273,8 +253,6 @@
// `fread` like functions where the size is computed as a multiplication of
// two arguments.
llvm::Optional<ArgNo> SizeMultiplierArgN;
- // The operator we use in apply. This is negated in negate().
- BinaryOperator::Opcode Op = BO_LE;
public:
StringRef getName() const override { return "BufferSize"; }
@@ -286,9 +264,9 @@
: ValueConstraint(Buffer), SizeArgN(BufSize),
SizeMultiplierArgN(BufSizeMultiplier) {}
- ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
- const Summary &Summary,
- CheckerContext &C) const override {
+ std::pair<ProgramStateRef, ProgramStateRef>
+ apply(ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
+ CheckerContext &C) const override {
SValBuilder &SvalBuilder = C.getSValBuilder();
// The buffer argument.
SVal BufV = getArgSVal(Call, getArgNo());
@@ -316,10 +294,10 @@
// The dynamic size of the buffer argument, got from the analyzer engine.
SVal BufDynSize = getDynamicSizeWithOffset(State, BufV);
- SVal Feasible = SvalBuilder.evalBinOp(State, Op, SizeV, BufDynSize,
+ SVal Feasible = SvalBuilder.evalBinOp(State, BO_LE, SizeV, BufDynSize,
SvalBuilder.getContext().BoolTy);
if (auto F = Feasible.getAs<DefinedOrUnknownSVal>())
- return State->assume(*F, true);
+ return State->assume(*F);
// We can get here only if the size argument or the dynamic size is
// undefined. But the dynamic size should never be undefined, only
@@ -330,12 +308,6 @@
llvm_unreachable("Size argument or the dynamic size is Undefined");
}
- ValueConstraintPtr negate() const override {
- BufferSizeConstraint Tmp(*this);
- Tmp.Op = BinaryOperator::negateComparisonOp(Op);
- return std::make_shared<BufferSizeConstraint>(Tmp);
- }
-
bool checkSpecificValidity(const FunctionDecl *FD) const override {
const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
assert(ValidArg &&
@@ -672,7 +644,8 @@
return State;
}
-ProgramStateRef StdLibraryFunctionsChecker::ComparisonConstraint::apply(
+std::pair<ProgramStateRef, ProgramStateRef>
+StdLibraryFunctionsChecker::ComparisonConstraint::apply(
ProgramStateRef State, const CallEvent &Call, const Summary &Summary,
CheckerContext &C) const {
@@ -690,8 +663,8 @@
OtherV = SVB.evalCast(OtherV, T, OtherT);
if (auto CompV = SVB.evalBinOp(State, Op, V, OtherV, CondT)
.getAs<DefinedOrUnknownSVal>())
- State = State->assume(*CompV, true);
- return State;
+ return State->assume(*CompV);
+ return {State, State};
}
void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
@@ -705,9 +678,9 @@
ProgramStateRef NewState = State;
for (const ValueConstraintPtr &Constraint : Summary.getArgConstraints()) {
- ProgramStateRef SuccessSt = Constraint->apply(NewState, Call, Summary, C);
- ProgramStateRef FailureSt =
- Constraint->negate()->apply(NewState, Call, Summary, C);
+ ProgramStateRef SuccessSt, FailureSt;
+ std::tie(SuccessSt, FailureSt) =
+ Constraint->apply(NewState, Call, Summary, C);
// The argument constraint is not satisfied.
if (FailureSt && !SuccessSt) {
if (ExplodedNode *N = C.generateErrorNode(NewState))
@@ -740,7 +713,8 @@
for (const ConstraintSet &Case : Summary.getCaseConstraints()) {
ProgramStateRef NewState = State;
for (const ValueConstraintPtr &Constraint : Case) {
- NewState = Constraint->apply(NewState, Call, Summary, C);
+ std::tie(NewState, std::ignore) =
+ Constraint->apply(NewState, Call, Summary, C);
if (!NewState)
break;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
