Eight StaticAnalyzer checker files define a REGISTER_CHECKER macro that
is never undefined, which can leak into subsequent translation units in
unity builds. Add #undef REGISTER_CHECKER at the end of each file. See
https://discourse.llvm.org/t/rfc-enabling-unity-build/90306 for more
info.
"clauded" not coded
Objective-C blocks are like lambdas. They have captures, just like lambdas.
However, they can also implicitly capture themselves unlike lambdas.
This means that when walking the captures of a block, we may end up in
infinite recursion. This is not possible with lambdas, but happened in
practice with blocks downstream.
In this patch, I just use a set to keep track of the visited MemRegions.
Note that theoretically, there is nothing preventing usual lambdas or
functors from falling for the same trap, but probably slightly more
difficult to do so. You would likely need a pointer to itself, etc. I'll
not speculate here.
This inf recursion was likely caused by #126620, released in clang-21.
rdar://162215172
This commit converts the class StackAddrEscapeChecker to the checker
family framework and slightly simplifies the implementation.
This commit is almost NFC, the only technically "functional" change is
that it removes the hidden modeling checker `core.StackAddrEscapeBase`
which was only relevant as an implementation detail of the old checker
registration procedure.
These are identified by misc-include-cleaner. I've filtered out those
that break builds. Also, I'm staying away from llvm-config.h,
config.h, and Compiler.h, which likely cause platform- or
compiler-specific build failures.
In general, if we see an allocation, we associate the immutable memory
space with the constructed memory region.
This works fine if we see the allocation.
However, with symbolic regions it's not great because there we don't
know anything about their memory spaces, thus put them into the Unknown
space.
The unfortunate consequence is that once we learn about some aliasing
with this Symbolic Region, we can't change the memory space to the
deduced one.
In this patch, we open up the memory spaces as a trait, basically
allowing associating a better memory space with a memregion that
was created with the Unknown memory space.
As a side effect, this means that now queriing the memory space of a
region depends on the State, but many places in the analyzer, such as
the Store, doesn't have (and cannot have) access to the State by design.
This means that some uses must solely rely on the memspaces of the
region, but any other users should use the getter taking a State.
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Fixes#123459.
This changes checking of the returned expr to also look for memory
regions whose stack frame context was a child of the current stack frame
context, e.g., for cases like this given in #123459:
```
struct S { int *p; };
S f() {
S s;
{
int a = 1;
s.p = &a;
}
return s;
}
```
Reapplying changes from https://github.com/llvm/llvm-project/pull/125638
after buildbot failures.
Buildbot failures fixed in 029e7e98dc9956086adc6c1dfb0c655a273fbee6,
latest commit on this PR. It was a problem with a declared class member
with same name as its type. Sorry!
Fixes https://github.com/llvm/llvm-project/issues/123459.
Previously, when the StackAddrEscapeChecker checked return values, it
did not scan into the structure of the return SVal. Now it does, and we
can catch some more false negatives that were already mocked out in the
tests in addition to those mentioned in
https://github.com/llvm/llvm-project/issues/123459.
The warning message at the moment for these newly caught leaks is not
great. I think they would be better if they had a better trace of why
and how the region leaks. If y'all are happy with these changes, I would
try to improve these warnings and work on normalizing this SVal checking
on the `checkEndFunction` side of the checker also.
Two of the stack address leak test cases now have two warnings, one
warning from return expression checking and another from`
checkEndFunction` `iterBindings` checking. For these two cases, I prefer
the warnings from the return expression checking, but I couldn't figure
out a way to drop the `checkEndFunction` without breaking other
`checkEndFunction` warnings that we do want. Thoughts here?
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]
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`.
Assigning to a pointer parameter does not leak the stack address because
it stays within the function and is not shared with the caller.
Previous implementation reported any association of a pointer parameter
with a local address, which is too broad.
This fix enforces that the pointer to a stack variable is related by at
least one level of indirection.
CPP-5642
Fixes#106834
As reported in
https://github.com/llvm/llvm-project/pull/105648#issuecomment-2317144635
commit 08ad8dc7154bf3ab79f750e6d5fb7df597c7601a
introduced a nullptr dereference in the case when store contains a
binding to a symbol that has no origin region associated with it, such
as the symbol generated when a pointer is passed to an opaque function.
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---------
This is the last of the 3 stacked PRs, it must not be merged before
https://github.com/llvm/llvm-project/pull/105652 and
https://github.com/llvm/llvm-project/pull/105653
This dump, if it is ever executed, is not actionable by the user and
might produce unwanted noise in the stderr.
The original intention behind this dump, to provide maximum information
in an unexpected situation, does not outweigh the potential annoyance
caused to users who might not even realize that they witnessed an
unexpected situation.
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output
parameters.
CPP-4734
-------
This is the first of three commits constituting
https://github.com/llvm/llvm-project/pull/105648
Basically, the issue was that we should have unwrapped the
base region before we special handle temp object regions.
Fixes https://github.com/llvm/llvm-project/issues/66221
I also decided to add some extra range information to the diagnostics
to make it consistent with the other reporting path.
...because it provides no useful functionality compared to its base
class `BugType`.
A long time ago there were substantial differences between `BugType` and
`BuiltinBug`, but they were eliminated by commit 1bd58233 in 2009 (!).
Since then the only functionality provided by `BuiltinBug` was that it
specified `categories::LogicError` as the bug category and it stored an
extra data member `desc`.
This commit sets `categories::LogicError` as the default value of the
third argument (bug category) in the constructors of BugType and
replaces use of the `desc` field with simpler logic.
Note that `BugType` has a data member `Description` and a non-virtual
method `BugType::getDescription()` which queries it; these are distinct
from the member `desc` of `BuiltinBug` and the identically named method
`BuiltinBug::getDescription()` which queries it. This confusing name
collision was a major motivation for the elimination of `BuiltinBug`.
As this commit touches many files, I avoided functional changes and left
behind FIXME notes to mark minor issues that should be fixed later.
Differential Revision: https://reviews.llvm.org/D158855
I'm involved with the Static Analyzer for the most part.
I think we should embrace newer language standard features and gradually
move forward.
Differential Revision: https://reviews.llvm.org/D154325
This patch introduces a new `CXXLifetimeExtendedObjectRegion` as a representation
of the memory for the temporary object that is lifetime extended by the reference
to which they are bound.
This separation provides an ability to detect the use of dangling pointers
(either binding or dereference) in a robust manner.
For example, the `ref` is conditionally dangling in the following example:
```
template<typename T>
T const& select(bool cond, T const& t, T const& u) { return cond ? t : u; }
int const& le = Composite{}.x;
auto&& ref = select(cond, le, 10);
```
Before the change, regardless of the value of `cond`, the `select()` call would
have returned a `temp_object` region.
With the proposed change we would produce a (non-dangling) `lifetime_extended_object`
region with lifetime bound to `le` or a `temp_object` region for the dangling case.
We believe that such separation is desired, as such lifetime extended temporaries
are closer to the variables. For example, they may have a static storage duration
(this patch removes a static temporary region, which was an abomination).
We also think that alternative approaches are not viable.
While for some cases it may be possible to determine if the region is lifetime
extended by searching the parents of the initializer expr, this quickly becomes
complex in the presence of the conditions operators like this one:
```
Composite cc;
// Ternary produces prvalue 'int' which is extended, as branches differ in value category
auto&& x = cond ? Composite{}.x : cc.x;
// Ternary produces xvalue, and extends the Composite object
auto&& y = cond ? Composite{}.x : std::move(cc).x;
```
Finally, the lifetime of the `CXXLifetimeExtendedObjectRegion` is tied to the lifetime of
the corresponding variables, however, the "liveness" (or reachability) of the extending
variable does not imply the reachability of all symbols in the region.
In conclusion `CXXLifetimeExtendedObjectRegion`, in contrast to `VarRegions`, does not
need any special handling in `SymReaper`.
RFC: https://discourse.llvm.org/t/rfc-detecting-uses-of-dangling-references/70731
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D151325
leaking in ARC mode
When ARC (automatic reference count) is enabled, (objective-c) block
objects are automatically retained and released thus they do not leak.
Without ARC, they still can leak from an expiring stack frame like
other stack variables.
With this commit, the static analyzer now puts a block object in an
"unknown" region if ARC is enabled because it is up to the
implementation to choose whether to put the object on stack initially
(then move to heap when needed) or in heap directly under ARC.
Therefore, the `StackAddrEscapeChecker` has no need to know
specifically about ARC at all and it will not report errors on objects
in "unknown" regions.
Reviewed By: NoQ (Artem Dergachev)
Differential Revision: https://reviews.llvm.org/D131009
A recent review emphasized the preference to use DefaultBool instead of
bool for checker options. This change is a NFC and cleans up some of the
instances where bool was used, and could be changed to DefaultBool.
Reviewed By: steakhal
Differential Revision: https://reviews.llvm.org/D123464
Not only global variables can hold references to dead stack variables.
Consider this example:
void write_stack_address_to(char **q) {
char local;
*q = &local;
}
void test_stack() {
char *p;
write_stack_address_to(&p);
}
The address of 'local' is assigned to 'p', which becomes a dangling
pointer after 'write_stack_address_to()' returns.
The StackAddrEscapeChecker was looking for bindings in the store which
referred to variables of the popped stack frame, but it only considered
global variables in this regard. This patch relaxes this, catching
stack variable bindings as well.
---
This patch also works for temporary objects like:
struct Bar {
const int &ref;
explicit Bar(int y) : ref(y) {
// Okay.
} // End of the constructor call, `ref` is dangling now. Warning!
};
void test() {
Bar{33}; // Temporary object, so the corresponding memregion is
// *not* a VarRegion.
}
---
The return value optimization aka. copy-elision might kick in but that
is modeled by passing an imaginary CXXThisRegion which refers to the
parent stack frame which is supposed to be the 'return slot'.
Objects residing in the 'return slot' outlive the scope of the inner
call, thus we should expect no warning about them - except if we
explicitly disable copy-elision.
Reviewed By: NoQ, martong
Differential Revision: https://reviews.llvm.org/D107078
Some checkers may not only depend on language options but also analyzer options.
To make this possible this patch changes the parameter of the shouldRegister*
function to CheckerManager to be able to query the analyzer options when
deciding whether the checker should be registered.
Differential Revision: https://reviews.llvm.org/D75271
Checkers are now required to specify whether they're creating a
path-sensitive report or a path-insensitive report by constructing an
object of the respective type.
This makes BugReporter more independent from the rest of the Static Analyzer
because all Analyzer-specific code is now in sub-classes.
Differential Revision: https://reviews.llvm.org/D66572
llvm-svn: 371450
Now that we've moved to C++14, we no longer need the llvm::make_unique
implementation from STLExtras.h. This patch is a mechanical replacement
of (hopefully) all the llvm::make_unique instances across the monorepo.
Differential revision: https://reviews.llvm.org/D66259
llvm-svn: 368942
This patch effectively fixes the almost decade old checker naming issue.
The solution is to assert when CheckerManager::getChecker is called on an
unregistered checker, and assert when CheckerManager::registerChecker is called
on a checker that is already registered.
Differential Revision: https://reviews.llvm.org/D55429
llvm-svn: 352292
Unfortunately, up until now, the fact that certain checkers depended on one
another was known, but how these actually unfolded was hidden deep within the
implementation. For example, many checkers (like RetainCount, Malloc or CString)
modelled a certain functionality, and exposed certain reportable bug types to
the user. For example, while MallocChecker models many many different types of
memory handling, the actual "unix.MallocChecker" checker the user was exposed to
was merely and option to this modeling part.
Other than this being an ugly mess, this issue made resolving the checker naming
issue almost impossible. (The checker naming issue being that if a checker
registered more than one checker within its registry function, both checker
object recieved the same name) Also, if the user explicitly disabled a checker
that was a dependency of another that _was_ explicitly enabled, it implicitly,
without "telling" the user, reenabled it.
Clearly, changing this to a well structured, declarative form, where the
handling of dependencies are done on a higher level is very much preferred.
This patch, among the detailed things later, makes checkers declare their
dependencies within the TableGen file Checkers.td, and exposes the same
functionality to plugins and statically linked non-generated checkers through
CheckerRegistry::addDependency. CheckerRegistry now resolves these dependencies,
makes sure that checkers are added to CheckerManager in the correct order,
and makes sure that if a dependency is disabled, so will be every checker that
depends on it.
In detail:
* Add a new field to the Checker class in CheckerBase.td called Dependencies,
which is a list of Checkers.
* Move unix checkers before cplusplus, as there is no forward declaration in
tblgen :/
* Add the following new checkers:
- StackAddrEscapeBase
- StackAddrEscapeBase
- CStringModeling
- DynamicMemoryModeling (base of the MallocChecker family)
- IteratorModeling (base of the IteratorChecker family)
- ValistBase
- SecuritySyntaxChecker (base of bcmp, bcopy, etc...)
- NSOrCFErrorDerefChecker (base of NSErrorChecker and CFErrorChecker)
- IvarInvalidationModeling (base of IvarInvalidation checker family)
- RetainCountBase (base of RetainCount and OSObjectRetainCount)
* Clear up and registry functions in MallocChecker, happily remove old FIXMEs.
* Add a new addDependency function to CheckerRegistry.
* Neatly format RUN lines in files I looked at while debugging.
Big thanks to Artem Degrachev for all the guidance through this project!
Differential Revision: https://reviews.llvm.org/D54438
llvm-svn: 352287
Introduce the boolean ento::shouldRegister##CHECKERNAME(const LangOptions &LO)
function very similarly to ento::register##CHECKERNAME. This will force every
checker to implement this function, but maybe it isn't that bad: I saw a lot of
ObjC or C++ specific checkers that should probably not register themselves based
on some LangOptions (mine too), but they do anyways.
A big benefit of this is that all registry functions now register their checker,
once it is called, registration is guaranteed.
This patch is a part of a greater effort to reinvent checker registration, more
info here: D54438#1315953
Differential Revision: https://reviews.llvm.org/D55424
llvm-svn: 352277
to reflect the new license.
We understand that people may be surprised that we're moving the header
entirely to discuss the new license. We checked this carefully with the
Foundation's lawyer and we believe this is the correct approach.
Essentially, all code in the project is now made available by the LLVM
project under our new license, so you will see that the license headers
include that license only. Some of our contributors have contributed
code under our old license, and accordingly, we have retained a copy of
our old license notice in the top-level files in each project and
repository.
llvm-svn: 351636
ClangCheckerRegistry is a very non-obvious, poorly documented, weird concept.
It derives from CheckerRegistry, and is placed in lib/StaticAnalyzer/Frontend,
whereas it's base is located in lib/StaticAnalyzer/Core. It was, from what I can
imagine, used to circumvent the problem that the registry functions of the
checkers are located in the clangStaticAnalyzerCheckers library, but that
library depends on clangStaticAnalyzerCore. However, clangStaticAnalyzerFrontend
depends on both of those libraries.
One can make the observation however, that CheckerRegistry has no place in Core,
it isn't used there at all! The only place where it is used is Frontend, which
is where it ultimately belongs.
This move implies that since
include/clang/StaticAnalyzer/Checkers/ClangCheckers.h only contained a single function:
class CheckerRegistry;
void registerBuiltinCheckers(CheckerRegistry ®istry);
it had to re purposed, as CheckerRegistry is no longer available to
clangStaticAnalyzerCheckers. It was renamed to BuiltinCheckerRegistration.h,
which actually describes it a lot better -- it does not contain the registration
functions for checkers, but only those generated by the tblgen files.
Differential Revision: https://reviews.llvm.org/D54436
llvm-svn: 349275
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
llvm-svn: 322753
The new check introduced in r318705 is useful, but suffers from a particular
class of false positives, namely, it does not account for
dispatch_barrier_sync() API which allows one to ensure that the asyncronously
executed block that captures a pointer to a local variable does not actually
outlive that variable.
The new check is split into a separate checker, under the name of
alpha.core.StackAddressAsyncEscape, which is likely to get enabled by default
again once these positives are fixed. The rest of the StackAddressEscapeChecker
is still enabled by default.
Differential Revision: https://reviews.llvm.org/D41042
llvm-svn: 320455
This diff extends StackAddrEscapeChecker
to catch stack addresses leaks via block captures
if the block is executed asynchronously or
returned from a function.
Differential revision: https://reviews.llvm.org/D39438
llvm-svn: 318705
Summary:
Leaking a stack address via a static variable refers to it in the diagnostic as a 'global'. This patch corrects the diagnostic for static variables.
Patch by Phil Camp, SN Systems
Reviewers: dcoughlin, zaks.anna
Subscribers: xazax.hun, cfe-commits
Differential Revision: http://reviews.llvm.org/D19866
Patch by Phil Camp
llvm-svn: 270849
Don't warn about addresses of stack-allocated blocks escaping if the block
region was cast with CK_CopyAndAutoreleaseBlockObject. These casts, which
are introduced in the implicit conversion operator for lambda-to-block
conversions, cause the block to be copied to the heap -- so the warning is
spurious.
llvm-svn: 254639
The analyzer trims unnecessary nodes from the exploded graph before reporting
path diagnostics. However, in some cases it can trim all nodes (including the
error node), leading to an assertion failure (see
https://llvm.org/bugs/show_bug.cgi?id=24184).
This commit addresses the issue by adding two new APIs to CheckerContext to
explicitly create error nodes. Unless the client provides a custom tag, these
APIs tag the node with the checker's tag -- preventing it from being trimmed.
The generateErrorNode() method creates a sink error node, while
generateNonFatalErrorNode() creates an error node for a path that should
continue being explored.
The intent is that one of these two methods should be used whenever a checker
creates an error node.
This commit updates the checkers to use these APIs. These APIs
(unlike addTransition() and generateSink()) do not take an explicit Pred node.
This is because there are not any error nodes in the checkers that were created
with an explicit different than the default (the CheckerContext's Pred node).
It also changes generateSink() to require state and pred nodes (previously
these were optional) to reduce confusion.
Additionally, there were several cases where checkers did check whether a
generated node could be null; we now explicitly check for null in these places.
This commit also includes a test case written by Ying Yi as part of
http://reviews.llvm.org/D12163 (that patch originally addressed this issue but
was reverted because it introduced false positive regressions).
Differential Revision: http://reviews.llvm.org/D12780
llvm-svn: 247859
The patch is generated using this command:
$ tools/extra/clang-tidy/tool/run-clang-tidy.py -fix \
-checks=-*,llvm-namespace-comment -header-filter='llvm/.*|clang/.*' \
work/llvm/tools/clang
To reduce churn, not touching namespaces spanning less than 10 lines.
llvm-svn: 240270
