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.
SValBuilder::getKnownValue, getMinValue, getMaxValue use
SValBuilder::simplifySVal.
simplifySVal does repeated simplification until a fixed-point is
reached. A single step is done by SimpleSValBuilder::simplifySValOnce,
using a Simplifier visitor. That will basically decompose SymSymExprs,
and apply constant folding using the constraints we have in the State.
Once it decomposes a SymSymExpr, it simplifies both sides and then uses
the SValBuilder::evalBinOp to reconstruct the same - but now simpler -
SymSymExpr, while applying some caching to remain performant.
This decomposition, and then the subsequent re-composition poses new
challenges to the SValBuilder::evalBinOp, which is built to handle
expressions coming from real C/C++ code, thus applying some implicit
assumptions.
One previous assumption was that nobody would form an expression like
"((int*)0) - q" (where q is an int pointer), because it doesn't really
makes sense to write code like that.
However, during simplification, we may end up with a call to evalBinOp
similar to this.
To me, simplifying a SymbolRef should never result in Unknown or Undef,
unless it was Unknown or Undef initially or, during simplification we
realized that it's a division by zero once we did the constant folding,
etc.
In the following case the simplified SVal should not become UnknownVal:
```c++
void top(char *p, char *q) {
int diff = p - q; // diff: reg<p> - reg<q>
if (!p) // p: NULL
simplify(diff); // diff after simplification should be: 0(loc) - reg<q>
}
```
Returning Unknown from the simplifySVal can weaken analysis precision in
other places too, such as in SValBuilder::getKnownValue, getMinValue, or
getMaxValue because we call simplifySVal before doing anything else.
For nonloc::SymbolVals, this loss of precision is critical, because for
those the SymbolRef carries an accurate type of the encoded computation,
thus we should at least have a conservative upper or lower bound that we
could return from getMinValue or getMaxValue - yet we would just return
nullptr.
```c++
const llvm::APSInt *SimpleSValBuilder::getKnownValue(ProgramStateRef state,
SVal V) {
return getConstValue(state, simplifySVal(state, V));
}
const llvm::APSInt *SimpleSValBuilder::getMinValue(ProgramStateRef state,
SVal V) {
V = simplifySVal(state, V);
if (const llvm::APSInt *Res = getConcreteValue(V))
return Res;
if (SymbolRef Sym = V.getAsSymbol())
return state->getConstraintManager().getSymMinVal(state, Sym);
return nullptr;
}
```
For now, I don't plan to make the simplification bullet-proof, I'm just
explaining why I made this change and what you need to look out for in
the future if you see a similar issue.
CPP-5750
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 generalizes the way element regions are constructed when an
`ArrayInitLoopExpr` is being analyzed. Previously the base region of the
`ElementRegion` was determined with pattern matching, which led to
crashes, when an unhandled pattern was encountered.
Fixes#112813
The current implementation of MemRegion::getDescriptiveName fails for
FieldRegions whose SuperRegion is an ElementRegion. As outlined below:
```Cpp
struct val_struct { int val; };
extern struct val_struct val_struct_array[3];
void func(){
// FieldRegion with ElementRegion as SuperRegion.
val_struct_array[0].val;
}
```
For this special case, the expression cannot be pretty printed and must
therefore be obtained separately.
When instantiating "callable<T>", the "class CallableType" nested type
will only have a declaration in the copy for the instantiation - because
it's not refereed to directly by any other code that would need a
complete definition.
However, in the past, when conservative eval calling member function, we
took the static type of the "this" expr, and looked up the CXXRecordDecl
it refereed to to see if it has any mutable members (to decide if it
needs to refine invalidation or not). Unfortunately, that query needs a
definition, and it asserts otherwise, thus we crashed.
To fix this, we should consult the dynamic type of the object, because
that will have the definition.
I anyways added a check for "hasDefinition" just to be on the safe side.
Fixes#77378
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.
This commit is a collection of several very minor code quality
improvements. The main goal is removing the misleading "Bin" substring
from the names of several methods and variables (like
`evalEagerlyAssumedBinOpBifurcation`) that are also applied for the
unary logical not operator.
In addition to this, I clarified the doc-comment of the method
`evalEagerlyAssumedBinOpBifurcation` and refactored the body of this
method to fix the capitalization of variable names and replace an
obsolete use of `std::tie` with a structured binding.
Finally, the data member `eagerlyAssumeBinOpBifurcation` of the class
`AnalyzerOptions` was completely removed (including a line in clang-tidy
that sets it to true), because it was never read by any code.
Note that the eagerly-assume mode of the analyzer is controlled by a
different boolean member of `AnalyzerOptions` which is called
`ShouldEagerlyAssume` and is defined via the macro magic from
`AnalyzerOptions.def`.
Ideally, we wouldn't workaround our current cast-modeling, but the
experimental "support-symbolic-integer-casts" is not finished so we need
to live with our current modeling.
Ideally, we could probably bind `UndefinedVal` as the result of the call
even without evaluating the call, as the result types mismatch between
the static type of the `CallExpr` and the actually function that happens
to be called.
Nevertheless, let's not crash.
https://compiler-explorer.com/z/WvcqK6MbY
CPP-5768
`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
The 'tile' clause shares quite a bit of the rules with 'collapse', so a
followup patch will add those tests/behaviors. This patch deals with
adding the AST node.
The 'tile' clause takes a series of integer constant expressions, or *.
The asterisk is now represented by a new OpenACCAsteriskSizeExpr node,
else this clause is very similar to others.
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).
Remove the declaration of `ErrnoChecker::checkBranchCondition()` because
this method is not defined or used anywhere. (It's probably a leftover
from some old refactoring.)
Tainted division operation is separated out from the core.DivideZero
checker into the optional optin.taint.TaintedDiv checker. The checker
warns when the denominator in a division operation is an attacker
controlled value.
This PR introduces new WebKit checker to warn a member variable that is
a raw reference or a raw pointer to an object, which is capable of
creating a CheckedRef/CheckedPtr.
This PR fixes a bug in UncountedCallArgsChecker that calling a function
with a member variable which is Ref/RefPtr is erroneously treated as
safe by canoniclizing the type before checking whether it's ref counted
or not.
There is no good way to tell CSA if function with `ownership_returns`
attribute returns initialized or not initialized memory. To make FP rate
lower, let's assume that memory returned from such functions is unknown
and do not reason about it.
In future it would be great to add a way to annotate such behavior
This reverts commit e39205654dc11c50bd117e8ccac243a641ebd71f.
There are further discussions in
https://github.com/llvm/llvm-project/pull/70976, happening for past two
weeks. Since there were no responses for couple weeks now, reverting
until author is back.
As was reported
[here](https://github.com/llvm/llvm-project/pull/103714#pullrequestreview-2238037812),
`invalidateRegions` should accept `Stmt` instead of `Expr`. This
conversion is possible, since `Expr` was anyway converted back to `Stmt`
later.
This refactoring is needed to fix another FP related to use of inline
assembly. The fix would be to change `State->bindLoc` to
`state->invalidateRegions` inside inline assembly visitor, since
`bindLoc` only binds to offset 0, which is not really correct semantics
in case of inline assembly.
Fixes#107852
Make it explicit that the checker skips `alloca` regions to avoid the
risk of producing false positives for code with advanced memory
management.
StackAddrEscapeChecker already used this strategy when it comes to
malloc'ed regions, so this change relaxes the assertion and explicitly
silents the issues related to memory regions generated with `alloca`.
As specified in the docs,
1) raw_string_ostream is always unbuffered and
2) the underlying buffer may be used directly
( 65b13610a5226b84889b923bae884ba395ad084d for further reference )
* Don't call raw_string_ostream::flush(), which is essentially a no-op.
* Avoid unneeded calls to raw_string_ostream::str(), to avoid excess indirection.
…ring.UninitRead
This is a drastic simplification of #106982. If you read that patch,
this is the same thing with all BugReporterVisitors.cpp and
SValBuilder.cpp changes removed! (since all replies came regarding
changed to those files, I felt the new PR was justified)
The patch was inspired by a pretty poor bug report on FFMpeg:
In this bug report, block is uninitialized, hence the bug report that it
should not have been passed to memcpy. The confusing part is in line 93,
where block was passed as a non-const pointer to seq_unpack_rle_block,
which was obviously meant to initialize block. As developers, we know
that clang likely didn't skip this function and found a path of
execution on which this initialization failed, but NoStoreFuncVisitor
failed to attach the usual "returning without writing to block" message.
I fixed this by instead of tracking the entire array, I tracked the
actual element which was found to be uninitialized (Remember, we
heuristically only check if the first and last-to-access element is
initialized, not the entire array). This is how the bug report looks
now, with 'seq_unpack_rle_block' having notes describing the path of
execution and lack of a value change:
Since NoStoreFuncVisitor was a TU-local class, I moved it back to
BugReporterVisitors.h, and registered it manually in CStringChecker.cpp.
This was done because we don't have a good trackRegionValue() function,
only a trackExpressionValue() function. We have an expression for the
array, but not for its first (or last-to-access) element, so I only had
a MemRegion on hand.
Current MalloChecker logic suppresses FP caused by refcounting only for
C++ destructors. The same pattern occurs a lot in C in objects with
intrusive refcounting. See #104229 for code example.
To extend current logic to C, suspect all release functions as candidate
for suppression.
Closes: #104229
PR refactors `MallocChecker` to not violate invariant of `BindExpr`,
which should be called only during `evalCall` to avoid conflicts.
To achieve this, most of `postCall` logic was moved to `evalCall` with
addition return value binding in case of processing of allocation
functions. Check functions prototypes was changed to use `State` with
bound return value.
`checkDelim` logic was left in `postCall` to avoid conflicts with
`StreamChecker` which also evaluates `getline` and friends.
PR also introduces breaking change in the unlikely case when the
definition of an allocation function (e.g. `malloc()`) is visible: now
checker does not try to inline allocation functions and assumes their
initial semantics.
Closes#73830
