In the current implementation, we run visitors until the fixed point is
reached.
That is, if a visitor adds another visitor, the currently processed path
is destroyed, all diagnostics is discarded, and it is regenerated again,
until it's no longer modified.
This pattern has a few negative implications:
- This loop does not even guarantee to terminate.
E.g. just imagine two visitors bouncing a diagnostics around.
- Performance-wise, e.g. for sqlite3 all visitors are being re-run at
least 10 times for some bugs.
We have already seen a few reports where it leads to timeouts.
- If we want to add more computationally intense visitors, this will
become worse.
- From architectural standpoint, the current layout requires copying
visitors, which is conceptually wrong, and can be annoying (e.g. no
unique_ptr on visitors allowed).
The proposed change is a much simpler architecture: the outer loop
processes nodes upwards, and whenever the visitor is added it only
processes current nodes and above, thus guaranteeing termination.
Differential Revision: https://reviews.llvm.org/D47856
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@335666 91177308-0d34-0410-b5e6-96231b3b80d8
ExprWithCleanups wraps full-expressions that require temporary destructors
and highlights the moment of time in which these destructors need to be called
(i.e., "at the end of the full-expression...").
Such expressions don't necessarily return an object; they may return anything,
including a null or undefined value.
When the analyzer tries to understand where the null or undefined value came
from in order to present better diagnostics to the user, it will now skip
any ExprWithCleanups it encounters and look into the expression itself.
Differential Revision: https://reviews.llvm.org/D48204
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@335559 91177308-0d34-0410-b5e6-96231b3b80d8
getEndPath is a problematic API, because it's not clear when it's called
(hint: not always at the end of the path), it crashes at runtime with
more than one non-nullptr returning implementation, and diagnostics
internal depend on it being called at some exact place.
However, most visitors don't actually need that: all they want is a
function consistently called after all nodes are traversed, to perform
finalization and to decide whether invalidation is needed.
Differential Revision: https://reviews.llvm.org/D48042
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@334540 91177308-0d34-0410-b5e6-96231b3b80d8
Summary: This is a prototype of a bug reporter visitor that invalidates bug reports by re-checking constraints of certain states on the bug path using the Z3 constraint manager backend. The functionality is available under the `crosscheck-with-z3` analyzer config flag.
Reviewers: george.karpenkov, NoQ, dcoughlin, rnkovacs
Reviewed By: george.karpenkov
Subscribers: rnkovacs, NoQ, george.karpenkov, dcoughlin, xbolva00, ddcc, mikhail.ramalho, MTC, fhahn, whisperity, baloghadamsoftware, szepet, a.sidorin, gsd, dkrupp, xazax.hun, cfe-commits
Differential Revision: https://reviews.llvm.org/D45517
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@333903 91177308-0d34-0410-b5e6-96231b3b80d8
When a '>>' token is split into two '>' tokens (in C++11 onwards), or (as an
extension) when we do the same for other tokens starting with a '>', we can't
just use a location pointing to the first '>' as the location of the split
token, because that would result in our miscomputing the length and spelling
for the token. As a consequence, for example, a refactoring replacing 'A<X>'
with something else would sometimes replace one character too many, and
similarly diagnostics highlighting a template-id source range would highlight
one character too many.
Fix this by creating an expansion range covering the first character of the
'>>' token, whose spelling is '>'. For this to work, we generalize the
expansion range of a macro FileID to be either a token range (the common case)
or a character range (used in this new case).
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@331155 91177308-0d34-0410-b5e6-96231b3b80d8
Summary: `TaintBugVisitor` is a universal visitor, and many checkers rely on it, such as `ArrayBoundCheckerV2.cpp`, `DivZeroChecker.cpp` and `VLASizeChecker.cpp`. Moving `TaintBugVisitor` to `BugReporterVisitors.h` enables other checker can also track where `tainted` value came from.
Reviewers: NoQ, george.karpenkov, xazax.hun
Reviewed By: george.karpenkov
Subscribers: szepet, rnkovacs, a.sidorin, cfe-commits, MTC
Differential Revision: https://reviews.llvm.org/D45682
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@330596 91177308-0d34-0410-b5e6-96231b3b80d8
Pointer arithmetic on null or undefined pointers results in null or undefined
pointers. This is obvious for undefined pointers; for null pointers it follows
from our incorrect-but-somehow-working approach that declares that 0 (Loc)
doesn't necessarily represent a pointer of numeric address value 0, but instead
it represents any pointer that will cause a valid "null pointer dereference"
issue when dereferenced.
For now we've been seeing through pointer arithmetic at the original dereference
expression, i.e. in bugreporter::getDerefExpr(), but not during further
investigation of the value's origins in bugreporter::trackNullOrUndefValue().
The patch fixes it.
Differential Revision: https://reviews.llvm.org/D45071
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@328896 91177308-0d34-0410-b5e6-96231b3b80d8
In the wild, many cases of null pointer dereference, or uninitialized
value read occur because the value was meant to be initialized by the
inlined function, but did not, most often due to error condition in the
inlined function.
This change highlights the return branch taken by the inlined function,
in order to help user understand the error report and see why the value
was uninitialized.
rdar://36287652
Differential Revision: https://reviews.llvm.org/D41848
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@325976 91177308-0d34-0410-b5e6-96231b3b80d8
We already suppress such reports for inlined functions, we should then
get the same behavior for macros.
The underlying reason is that the same macro, can be called from many
different contexts, and nullability can only be expected in _some_ of
them.
Assuming that the macro can return null in _all_ of them sometimes leads
to a large number of false positives.
E.g. consider the test case for the dynamic cast implementation in
macro: in such cases, the bug report is unwanted.
Tracked in rdar://36304776
Differential Revision: https://reviews.llvm.org/D42404
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@324161 91177308-0d34-0410-b5e6-96231b3b80d8
In most cases using
`N->getState()->getSVal(E, N->getLocationContext())`
is ugly, verbose, and also opens up more surface area for bugs if an
inconsistent location context is used.
This patch introduces a helper on an exploded node, and ensures
consistent usage of either `ExplodedNode::getSVal` or
`CheckContext::getSVal` across the codebase.
As a result, a large number of redundant lines is removed.
Differential Revision: https://reviews.llvm.org/D42155
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@322753 91177308-0d34-0410-b5e6-96231b3b80d8
The current code used to not suppress the report, if the dereference was
performed in a macro, assuming it is that same macro.
However, the assumption might not be correct, and XNU has quite a bit of
code where dereference is actually performed in a different macro.
As the code uses macro name and not a unique identifier it might be fragile,
but in a worst-case scenario we would simply emit an extra diagnostic.
rdar://36160245
Differential Revision: https://reviews.llvm.org/D41749
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@322149 91177308-0d34-0410-b5e6-96231b3b80d8
When trying to figure out where a null or undefined value came from,
parentheses and cast expressions are either completely irrelevant, or,
in the case of lvalue-to-rvale cast, straightforwardly lead us in the right
direction when we remove them.
There is a regression that causes a certain diagnostic to appear twice in the
path-notes.cpp test (changed to FIXME). It would be addressed in the next
commit.
Differential revision: https://reviews.llvm.org/D41254
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@321133 91177308-0d34-0410-b5e6-96231b3b80d8
When reporting certain kinds of analyzer warnings, we use the
bugreporter::trackNullOrUndefValue mechanism, which is part of public checker
API, to understand where a zero, null-pointer, or garbage value came from,
which would highlight important events with respect to that value in the
diagnostic path notes, and help us suppress various false positives that result
from values appearing from particular sources.
Previously, we've lost track of the value when it was written into a memory
region that is not a plain variable. Now try to resume tracking in this
situation by finding where the last write to this region has occured.
Differential revision: https://reviews.llvm.org/D41253
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@321130 91177308-0d34-0410-b5e6-96231b3b80d8
Adding the new enumerator forced a bunch more changes into this patch than I
would have liked. The -Wtautological-compare warning was extended to properly
check the new comparison operator, clang-format needed updating because it uses
precedence levels as weights for determining where to break lines (and several
operators increased their precedence levels with this change), thread-safety
analysis needed changes to build its own IL properly for the new operator.
All "real" semantic checking for this operator has been deferred to a future
patch. For now, we use the relational comparison rules and arbitrarily give
the builtin form of the operator a return type of 'void'.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@320707 91177308-0d34-0410-b5e6-96231b3b80d8
This function can now track null pointer through simple pointer arithmetic,
such as '*&*(p + 2)' => 'p' and so on, displaying intermediate diagnostic pieces
for the user to understand where the null pointer is coming from.
Differential Revision: https://reviews.llvm.org/D37025
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@314290 91177308-0d34-0410-b5e6-96231b3b80d8
This API is used by checkers (and other entities) in order to track where does
a value originate from, by jumping from an expression value of which is equal
to that value to the expression from which this value has "appeared". For
example, it may be an lvalue from which the rvalue was loaded, or a function
call from which the dereferenced pointer was returned.
The function now avoids incorrectly unwrapping implicit lvalue-to-rvalue casts,
which caused crashes and incorrect intermediate diagnostic pieces. It also no
longer relies on how the expression is written when guessing what it means.
Fixes pr34373 and pr34731.
rdar://problem/33594502
Differential Revision: https://reviews.llvm.org/D37023
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@314287 91177308-0d34-0410-b5e6-96231b3b80d8
Array-to-pointer cast now works correctly when the pointer to the array
is concrete, eg. null, which allows further symbolic calculations involving
such values.
Inlined defensive checks are now detected correctly when the resulting null
symbol is being array-subscripted before dereference.
Differential Revision: https://reviews.llvm.org/D32291
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@301251 91177308-0d34-0410-b5e6-96231b3b80d8
Null dereferences are suppressed if the lvalue was constrained to 0 for the
first time inside a sub-function that was inlined during analysis, because
such constraint is a valid defensive check that does not, by itself,
indicate that null pointer case is anyhow special for the caller.
If further operations on the lvalue are performed, the symbolic lvalue is
collapsed to concrete null pointer, and we need to track where does the null
pointer come from.
Improve such tracking for lvalue operations involving operator &.
rdar://problem/27876009
Differential Revision: https://reviews.llvm.org/D31982
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@301224 91177308-0d34-0410-b5e6-96231b3b80d8
Simplifies and makes explicit the memory ownership model rather than
implicitly passing/acquiring ownership.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@291143 91177308-0d34-0410-b5e6-96231b3b80d8
When a macro expending to a literal is used in a comparison, use the macro name
in the diagnostic rather than the literal. This improves readability of path
notes.
Added tests for various macro literals that could occur. Only BOOl, Int, and
NULL tests have changed behavior with this patch.
Differential Revision: https://reviews.llvm.org/D27726
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@289884 91177308-0d34-0410-b5e6-96231b3b80d8
Logical short-circuit operators now act like other branch conditions.
If the symbolic value of the left-hand side is not known to be true or false
(based on the previous execution path), the "Assuming" event piece is added
in order to explain that the analyzer is adding a new assumption.
Additionally, when the assumption is made against the right-hand side of
the logical operator (i.e. when the operator itself acts as a condition
in another CFG terminator), the "Assuming..." piece is written out for the
right-hand side of the operator rather than for the whole operator.
This allows expression-specific diagnostic message text to be constructed.
Differential Revision: https://reviews.llvm.org/D25092
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@283302 91177308-0d34-0410-b5e6-96231b3b80d8
In the analyzer's path-sensitive reports, when a report goes through a branch
and the branch condition cannot be decided to be definitely true or false
(based on the previous execution path), an event piece is added that tells the
user that a new assumption is added upon the symbolic value of the branch
condition. For example, "Assuming 'a' is equal to 3".
The text of the assumption is hand-crafted in various manners depending on
the AST expression. If the AST expression is too complex and the text of
the assumption fails to be constructed, the event piece is omitted.
This causes loss of information and misunderstanding of the report.
Do not omit the event piece even if the expression is too complex;
add a piece with a generic text instead.
Differential Revision: https://reviews.llvm.org/D23300
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@283301 91177308-0d34-0410-b5e6-96231b3b80d8
We should ignore paren casts when making sure that the semantic expression
in a PseudoObjectExpr for an ObjC getter is a message send.
This has no other intended functionality change.
Adding a test for this exposed an interesting issue in another test case
that only manifests under ARC. trackNullOrUndefValue() is not properly
suppressing for nil values that are the result of nil propagation from a nil
receiver when the nil is returned from a function. I've added a FIXME for that
missing suppression.
rdar://problem/27290568
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@279181 91177308-0d34-0410-b5e6-96231b3b80d8
This checker checks copy and move assignment operators whether they are
protected against self-assignment. Since C++ core guidelines discourages
explicit checking for `&rhs==this` in general we take a different approach: in
top-frame analysis we branch the exploded graph for two cases, where &rhs==this
and &rhs!=this and let existing checkers (e.g. unix.Malloc) do the rest of the
work. It is important that we check all copy and move assignment operator in top
frame even if we checked them already since self-assignments may happen
undetected even in the same translation unit (e.g. using random indices for an
array what may or may not be the same).
This reapplies r275820 after fixing a string-lifetime issue discovered by the
bots.
A patch by Ádám Balogh!
Differential Revision: https://reviews.llvm.org/D19311
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@276365 91177308-0d34-0410-b5e6-96231b3b80d8
This checker checks copy and move assignment operators whether they are
protected against self-assignment. Since C++ core guidelines discourages
explicit checking for `&rhs==this` in general we take a different approach: in
top-frame analysis we branch the exploded graph for two cases, where &rhs==this
and &rhs!=this and let existing checkers (e.g. unix.Malloc) do the rest of the
work. It is important that we check all copy and move assignment operator in top
frame even if we checked them already since self-assignments may happen
undetected even in the same translation unit (e.g. using random indices for an
array what may or may not be the same).
A patch by Ádám Balogh!
Differential Revision: https://reviews.llvm.org/D19311
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@275820 91177308-0d34-0410-b5e6-96231b3b80d8
The analyzer does not model C++ temporary destructors completely and so
reports false alarms about leaks of memory allocated by the internals of
shared_ptr:
std::shared_ptr<int> p(new int(1));
p = nullptr; // 'Potential leak of memory pointed to by field __cntrl_'
This patch suppresses all diagnostics where the end of the path is inside
a method in std::shared_ptr.
It also reorganizes the tests for suppressions in the C++ standard library
to use a separate simulated header for library functions with bugs
that were deliberately inserted to test suppression. This will prevent
other tests from using these as models.
rdar://problem/23652766
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@274691 91177308-0d34-0410-b5e6-96231b3b80d8
Teach trackNullOrUndefValue() how to properly look through PseudoObjectExprs
to find the underlying semantic method call for property getters. This fixes a
crash when looking through class property getters that I introduced in r265839.
rdar://problem/26796666
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@273340 91177308-0d34-0410-b5e6-96231b3b80d8
Don't emit a path note marking the return site if the return statement does not
have a valid location. This fixes an assertion failure I introduced in r265839.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@266031 91177308-0d34-0410-b5e6-96231b3b80d8
Teach trackNullOrUndefValue() how to look through PseudoObjectExprs to find
the underlying method call for property getters. This makes over-suppression
of 'return nil' in getters consistent with the similar over-suppression for
method and function calls.
rdar://problem/24437252
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@265839 91177308-0d34-0410-b5e6-96231b3b80d8
Now that the libcpp implementations of these methods has a branch that doesn't call
memmove(), the analyzer needs to invalidate the destination for these methods explicitly.
rdar://problem/23575656
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@260043 91177308-0d34-0410-b5e6-96231b3b80d8
This is the default for the analyzer but the flag is added by the driver so
our suppression tests didn't cover this case.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@259288 91177308-0d34-0410-b5e6-96231b3b80d8
We already do this for case splits introduced as a result of defensive null
checks in functions and methods, so do the same for function-like macros.
rdar://problem/19640441
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@259222 91177308-0d34-0410-b5e6-96231b3b80d8
The analyzer reports a shift by a negative value in the constructor. The bug can
be easily triggered by calling std::random_shuffle on a vector
(<rdar://problem/19658126>).
(The shift by a negative value is reported because __w0_ gets constrained to
63 by the conditions along the path:__w0_ < _WDt && __w0_ >= _WDt-1,
where _WDt is 64. In normal execution, __w0_ is not 63, it is 1 and there is
no overflow. The path is infeasible, but the analyzer does not know about that.)
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@256886 91177308-0d34-0410-b5e6-96231b3b80d8
George Karpenkov