The checker currently reports beneath the null dereference dereferences
of undefined value and of label addresses. If we want to add more kinds
of invalid dereferences (or split the existing functionality) it is more
useful to make it separate checkers.
To make this possible the existing checker is split into a
DereferenceModeling part and a NullDereference checker that actually
only switches on the check of null dereference. This is similar
architecture as in MallocChecker and CStringChecker.
The change is almost NFC but a new (modeling) checker is added. If the
NullDereference checker is turned off the found invalid dereferences
will still stop the analysis without emitted warning (this is different
compared to the old behavior).
In WebKit, we often capture this as Ref or RefPtr in addition to this
itself so that the object lives as long as a capturing lambda stays
alive.
Detect this pattern and treat it as safe. This PR also makes the check
for a lambda being passed as a function argument more robust by handling
CXXBindTemporaryExpr, CXXConstructExpr, and DeclRefExpr referring to the
lambda.
adoptRef in WebKit constructs Ref/RefPtr so treat it as such in
isCtorOfRefCounted.
Also removed the support for makeRef and makeRefPtr as they don't exist
any more.
adoptRef in WebKit constructs Ref/RefPtr so treat it as such in
isCtorOfRefCounted. Also removed the support for makeRef and makeRefPtr
as they don't exist any more.
One could create dangling APSInt references in various ways in the past, that were sometimes assumed to be persisted in the BasicValueFactor.
One should always use BasicValueFactory to create persistent APSInts, that could be used by ConcreteInts or SymIntExprs and similar long-living objects.
If one used a temporary or local variables for this, these would dangle.
To enforce the contract of the analyzer BasicValueFactory and the uses of APSInts, let's have a dedicated strong-type for this.
The idea is that APSIntPtr is always owned by the BasicValueFactory, and that is the only component that can construct it.
These PRs are all NFC - besides fixing dangling APSInt references.
In WebKit, we often write Foo::ensureBar function which lazily
initializes m_bar and returns a raw pointer or a raw reference to m_bar.
Such a return value is safe to use for the duration of a member function
call in Foo so long as m_bar is const so that it never gets unset or
updated with a new value once it's initialized.
This PR adds support for recognizing these types of functions and
treating its return value as a safe origin of a function argument
(including "this") or a local variable.
It's safe to have a raw pointer or a raw reference to a singleton
object. Explicitly allow this in UncountedLocalVarsChecker and
UncheckedLocalVarsChecker.
This PR introduces a new checker
`[alpha.webkit.MemoryUnsafeCastChecker]` that warns all downcasts from a base type to a derived type.
rdar://137766829
Note that PointerUnion::{is,get} have been soft deprecated in
PointerUnion.h:
// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa<T>, cast<T> and the llvm::dyn_cast<T>
I'm not touching PointerUnion::dyn_cast for now because it's a bit
complicated; we could blindly migrate it to dyn_cast_if_present, but
we should probably use dyn_cast when the operand is known to be
non-null.
This change removes the alpha.core.IdenticalExpr static analysis checker
since it's checks are present in the clang-tidy checks
misc-redundant-expression and bugprone-branch-clone. This check was
implemented as a static analysis check using AST matching, and since
alpha and duplicated in 2 clang-tidy checks may be removed.
Co-authored-by: Vince Bridgers <vince.a.bridgers@ericsson.com>
This change modernizes, improves and promotes the chroot checker from
alpha to the Unix family of checkers. This checker covers the POS05
recommendations for use of chroot.
The improvements included modeling of a success or failure from chroot
and not falsely reporting a warning along an error path. This was made
possible through modernizing the checker to be flow sensitive.
---------
Co-authored-by: einvbri <vince.a.bridgers@ericsson.com>
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Fixed a bug that UncountedLambdaCapturesChecker would emit a warning on
a lambda capture when the lambda is invoked with arguments.
LocalVisitor::VisitCallExpr was not tolerating a lambda invocation with
more than 1 arguments.
returned by-value from opaque function calls.
If a struct is returned by-value from an opaque call, the "value" of the
whole struct is represented by a Conjured symbol.
Later fields may slice off smaller subregions by creating Derived
symbols of that Conjured symbol, but those are handled well, and
"isTainted" returns true as expected.
However, passing the whole struct to "isTainted" would be false, because
LazyCompoundVals and CompoundVals are not handled.
This patch addresses this.
Fixes#114270
Split from #114835
This could deduce some complex syms derived from simple ones whose
values could be constrainted to be concrete during execution, thus
reducing some overconstrainted states.
This commit also fix `unix.StdCLibraryFunctions` crash due to these
overconstrainted states being added to the graph, which is marked as
sink node (PosteriorlyOverconstrained). The 'assume' API is used in
non-dual style so the checker should protectively test whether these
newly added nodes are actually impossible.
1. The crash: https://godbolt.org/z/8KKWeKb86
2. The solver needs to solve equivalent: https://godbolt.org/z/ed8WqsbTh
Treat member function calls to ref() and incrementCheckedPtrCount() as
trivial so that a function which returns RefPtr/Ref out of a raw
reference / pointer is also considered trivial.
This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial
functions as well as the one being passed to an argument with
[[clang::noescape]] attribute. This dramatically reduces the false
positive rate for this checker.
To do this, this PR replaces VisitLambdaExpr in favor of checking
lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a
lambda is defined but never called or stored somewhere, then capturing
whatever variable in such a lambda is harmless.
VisitCallExpr explicitly looks for direct invocation of lambdas and
registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda
is being passed to a function, it checks whether its argument is
annotated with [[clang::noescape]]. If it's not annotated such, it
checks captures for their safety.
Because WTF::switchOn could not be annotated with [[clang::noescape]] as
function type parameters are variadic template function so we hard-code
this function into the checker.
In order to check whether "this" pointer is ref-counted type or not, we
override TraverseDecl and record the most recent method's declaration.
In addition, this PR fixes a bug in isUnsafePtr that it was erroneously
checking whether std::nullopt was returned by isUncounted and
isUnchecked as opposed to the actual boolean value.
Finally, this PR also converts the accompanying test to use -verify and
adds a bunch of tests.
This PR introduces alpha.webkit.UncheckedCallArgsChecker which detects a
function argument which is a raw reference or a raw pointer to a
CheckedPtr capable object.
This commit removes the `check::BranchCondition` callback of the debug
checker `debug.DumpTraversal` (in `TraversalChecker.cpp`) and the single
broken testcase that was referring to it.
The testcase `traversal-algorithm.mm` was added in 2012 to verify that
we're using DFS traversal -- however it failed to detect that we're no
longer using DFS traversal and in fact it continues to pass even if I
remove large random portions of its code.
This change was motivated by the plan discussed at
https://discourse.llvm.org/t/fixing-or-removing-check-branchcondition/82738
I also added some TODO notes to mark the rest of `TraversalChecker.cpp`
for removal in follow-up commits.
This PR introduces alpha.webkit.UncheckedLocalVarsChecker which detects
a raw reference or a raw pointer local, static, or global variable to a
CheckedPtr capable object without a guardian variable in an outer scope.
Fixes#62985Fixes#58820
When 3rd-party header files are included as system headers, their
overloaded `new` and `delete` operators are also considered as the std
ones. However, those overloaded operator functions will also be inlined.
This makes the same
symbolic memory marked as released twice: during `checkPreCall` of the
overloaded `delete` operator and when calling `::operator delete` after
inlining the overloaded operator function (if it has).
This patch attempts to fix this bug by adjusting the strategy of
verifying whether the callee is a standard `new` or `delete` operator in
the `isStandardNewDelete` function.
This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial
functions as well as the one being passed to an argument with
[[clang::noescape]] attribute. This dramatically reduces the false
positive rate for this checker.
To do this, this PR replaces VisitLambdaExpr in favor of checking
lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a
lambda is defined but never called or stored somewhere, then capturing
whatever variable in such a lambda is harmless.
VisitCallExpr explicitly looks for direct invocation of lambdas and
registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda
is being passed to a function, it checks whether its argument is
annotated with [[clang::noescape]]. If it's not annotated such, it
checks captures for their safety.
Because WTF::switchOn could not be annotated with [[clang::noescape]] as
function type parameters are variadic template function so we hard-code
this function into the checker.
Finally, this PR also converts the accompanying test to use -verify and
adds a bunch of tests.
This checker has a notion of a guardian variable which is a variable and
keeps the object pointed to by a raw pointer / reference in an inner
scope alive long enough to "guard" it from use-after-free. But such a
guardian variable fails to flawed to keep the object alive if it ever
gets mutated within the scope of a raw pointer / reference.
This PR fixes this bug by introducing a new AST visitor class,
GuardianVisitor, which traverses the compound statements of a guarded
variable (raw pointer / reference) and looks for any operator=, move
constructor, or calls to "swap", "leakRef", or "releaseNonNull"
functions.
This change moves the `alpha.nondeterministic.PointerSorting` and
`alpha.nondeterministic.PointerIteration` static analyzer checkers to a
single `clang-tidy` check. Those checkers were implemented as simple
`clang-tidy` check-like code, wrapped in the static analyzer framework.
The documentation was updated to describe what the checks can and cannot
do, and testing was completed on a broad set of open-source projects.
Co-authored-by: Vince Bridgers <vince.a.bridgers@ericsson.com>
This patch simplifies the diagnostic message in the core.StackAddrEscape
for stack memory associated with compound literals by removing the
redundant "returned to caller" suffix.
Example: https://godbolt.org/z/KxM67vr7c
```c
// clang --analyze -Xanalyzer -analyzer-checker=core.StackAddressEscape
void* compound_literal() {
return &(unsigned short){((unsigned short)0x22EF)};
}
```
warning: Address of stack memory associated with a compound literal
declared on line 2 **returned to caller returned to caller**
[core.StackAddressEscape]
This PR fixes the bug that alpha.webkit.UncountedLocalVarsChecker
erroneously treats a trivial recursive function as non-trivial. This was
caused by TrivialFunctionAnalysis::isTrivialImpl which takes a statement
as an argument populating the cache with "false" while traversing the
statement to determine its triviality within a recursive function in
TrivialFunctionAnalysisVisitor's WithCachedResult. Because
IsFunctionTrivial honors an entry in the cache, this resulted in the
whole function to be treated as non-trivial.
Thankfully, TrivialFunctionAnalysisVisitor::IsFunctionTrivial already
handles recursive functions correctly so this PR applies the same logic
to TrivialFunctionAnalysisVisitor::WithCachedResult by sharing code
between the two functions. This avoids the cache to be pre-populated
with "false" while traversing statements in a recurisve function.
`builtin_*_overflow` functions return `_Bool` according to [1].
`BuiltinFunctionChecker` was using `makeTruthVal` w/o specifying
explicit type, which creates an `int` value, since it's the type of any
compassion according to C standard.
Fix it by directly passing `BoolTy` to `makeTruthVal`
Closes: #111147
[1]
https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins
Add basic support for `builtin_*_overflow` primitives.
These helps a lot for checking custom calloc-like functions with
inlinable body. Without such support code like
```c
#include <stddef.h>
#include <stdlib.h>
static void *myMalloc(size_t a1, size_t a2)
{
size_t res;
if (__builtin_mul_overflow(a1, a2, &res))
return NULL;
return malloc(res);
}
void test(void)
{
char *ptr = myMalloc(10, 1);
ptr[20] = 10;
}
````
does not trigger any warnings.
If a fixed value is assigned to a pointer variable, the checker did emit
a warning. If the pointer variable is assigned to another pointer
variable, this resulted in another warning. The checker now emits
warning only if a value with non-pointer type is assigned (to a pointer
variable).
