| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
(#161370)
Downstream, some change triggered an investigation if we could move a
checker callback from check::PostCall to eval::Call. After a lengthy
investigation that lead to ExprEngine::VisitCXXNewExpr we realized that
CXXNewExprs only trigger a PreCall and PostCall, but never an EvalCall.
It also had a FIXME that maybe it should trigger it.
Remember, it called `defaultEvalCall` which either inlines or
conservatively evaluates aka. invalidates the call. But never probes the
checker eval-calls to see if any would step in.
After implementing the changes to trigger the eval call for the
checkers, I realized that it doesn't really make sense because we are
eval-calling user-provided functions, that we can't be really sure about
their semantics, thus there is no generic way to properly implement the
eval call callback.
This touches on an important point. It only ever makes sense to eval
call functions that has a clear spec. such as standard functions, as
implementing the callback would prevent the inlining of that function,
risking regressing analysis quality if the implemented model is not
complete/correct enough.
As a conclusion, I opted for not exposing the eval call event to
checkers, in other words, keep everything as-is, but document my
journey.
CPP-6585
|
|
not pointers (#160511)
https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp#L675-L678
mistakenly assumes that target expressions are of pointer type.
`CheckOverlap` has multiple call sites, most of which do not verify this
assumption. Therefore, the simplest solution is to verify it just before
that point.
|
|
@implementation is seen (#159947)
A @interface declaration with a raw pointer @property does not
necessarily mean it synthesizes ivar of that type. To determine whether
such a synthesis happens or not, we must wait for @implementation to
appear. So this PR makes the checker only validate @property then.
|
|
|
|
This PR ensures that the Clang static analyzer loads the config file
through the properly-configured VFS rather than through the bare real
file system. This enables correctly going through VFS overlays, unifying
the behavior with the rest of the compiler.
|
|
|
|
Add the support for OSObjectPtr, which behaves like RetainPtr.
|
|
std::ranges::all_of as [[clang::noescape]] (#158419)
The checker already had std::ranges hard-coded to treat its arguments as
[[clang::oescape]] but the fact std::ranges::all_of is implemented as a
struct instead of a function confused the checker and resuled in a
superflous warning being emitted for std::ranges::all_of.
This PR adds the support for recognizing DeclRefExpr which appears as a
callee in VisitCallExpr and generalizes the check in
shouldTreatAllArgAsNoEscape to walk up the decl contexts to find the
target namespaces such as std::ranges:: or a namespace and a function
like WTF::switchOn.
|
|
The underflow reports of checker security.ArrayBound previously
displayed the (negative) byte offset of the accessed location; but those
numbers were a bit hard to decipher (especially when the elements were
large structs), so this commit replaces the byte offset with an index
value (if the offset is an integer multiple of the size of the accessed
element).
This change only affects the content of the messages; the checker will
find the same issues before and after this commit.
|
|
singleton. (#158012)
|
|
treated as unsafe if it's a template parameter (#157993)
When a template class takes Ref<T> as a template parameter and this
template parameter is used as the return value of a member function, the
return value can be treated as unsafe (i.e. emits a false positive). The
issue was caused by getCanonicalType sometimes converting Ref<T> to T.
Workaround this problem by avoid emitting a warning when the original,
non-canonical type is a safe pointer type.
|
|
Like other functions which results in abort, treat asm brk instruction
as trivial.
|
|
(#157629)
This PR adds the support for treating a function return value to be safe
if the function is annotated with NS_RETURNS_RETAINED or
CF_RETURNS_RETAINED.
|
|
Previously the checker `security.VAList` only tracked the set of the
inintialized `va_list` objects; this commit replaces this with a mapping
that can distinguish the "uninitialized" `va_list` objects from the
"already released" ones. Moreover, a new "unknown" state is introduced
to replace the slightly hacky solutions that checked the `Symbolic`
nature of the region.
In addition to sligthly improving the messages, this commit also
prepares the ground for a follow-up change that would introduce an
"indeterminate" state (which needs `va_end` but cannot be otherwise
used) to model the requirements of SEI CERT rule MSC39-C, which states:
> The va_list may be passed as an argument to another function, but
> calling va_arg() within that function causes the va_list to have an
> indeterminate value in the calling function. As a result, attempting
> to read variable arguments without reinitializing the va_list can have
> unexpected behavior.
|
|
This PR makes WebKit checkers treat NULL, 0, and nil like nullptr in
various places.
|
|
no-escape argument (#155025)
Fix a bug that webkit.UncountedLambdaCapturesChecker was erroneously
emitting a warning for a DeclRefExpr which is passed in as an argument
to a no-escape function argument. The bug was caused by findLambdaInArg
not adding DeclRefExpr to the ignored set even when a lambda was
identified as an argument.
|
|
temporary object as an argument. (#155033)
Since a temporary object lives until the end of the statement, it's safe
to pass such an object as a function argument without explicitly
creating a CheckedRef/CheckedPtr in stack.
|
|
...to follow the capitalization style that was already applied within
the file by recent commit a80c393a9c498279a1ec9fd630535b9ff139b49f.
|
|
Previously the analyzer had an undocumented top-level checker group
called `valist` which offered several checkers to detect use of
uninitialized `va_list` objects and leaks of `va_list`s.
As the responsibilities of these checkers were messily intertwined and
`va_list` is a rarely used language feature, this commit simplifies the
situation by consolidating these checkers into a single checker which
will be called `security.VAList`.
Note that I'm choosing the capitalization `VAList` to be consistent with
the example of the AST node type `VAArgExpr`. I updated many variable
names to ensure that `ValistChecker.cpp` uses this spelling everywhere
(in CamelCased names). I'm planning to rename `ValistChecker.cpp` to
`VAListChecker.cpp` in a follow-up commit.
This commit also adds documentation for this checker in checkers.rst.
Among the test files I preserved the existing separation but I
eliminated some duplicated cases now that there is no way to separately
enable the old sub-checkers.
For the background of this change see also the discourse thread
https://discourse.llvm.org/t/clean-up-valist-checkers/85277/3
|
|
This pull improves the handling of placement new in`PointerArith`,
fixing one family of false positives, and one of negatives:
### False Positives
```cpp
Buffer buffer;
int* array = new (&buffer) int[10];
++array; // there should be no warning
```
The code above should flag the memory region `buffer` as reinterpreted,
very much as `reinterpret_cast` would do. Note that in this particular
case the placement new is inlined so the engine can track that `*array`
points to the same region as `buffer`.
This is no-op if the placement new is opaque.
### False Negatives
```cpp
Buffer buffer;
int* array = new (&buffer) int;
++array; // there should be a warning
```
In this case, there is an implicit cast to `void*` when calling
placement new. The memory region was marked as reinterpreted, and
therefore later pointer arithmetic will not raise. I have added a
condition to not consider a cast to `void*` as a reinterpretation, as an
array of voids does not make much sense.
There are still some limitations, of course. For starters, if a single
`int` is created in place of an array of `unsigned char` of exactly the
same size, it will still be considered as an array. A convoluted example
to make the point that I think it makes sense *not* to raise in this
situation is in the test `checkPlacementNewSlices`.
CPP-6868
|
|
Signature:
```c
size_t strxfrm(char *dest, const char *src, size_t n);
```
The modeling covers:
* `src` can never be null
* `dest` can be null only if n is 0, and then the return value is some unspecified positive integer
* `src` and `dest` must not overlap
* `dest` must have at least `n` bytes of capacity
* The return value can either be:
- `< n`, and the contents of the buffer pointed by `dest` is invalidated
- `>= n`, and the contents of the buffer pointed by `dest` is marked as undefined
CPP-6854
|
|
temporary objects (#152751)
Fixes PR60896 - false positive leak reports in various smart pointer scenarios including temporaries, inheritance, direct constructor calls, and mixed ownership patterns. Previously, the analyzer had no smart pointer handling in
`checkPostCall`, causing it to report false positive leaks for memory properly managed by smart pointers while missing legitimate raw pointer leaks.
---------
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Co-authored-by: Donát Nagy <donat.nagy@ericsson.com>
|
|
In CallAndMessageChecker the initialization of bug types was highly
obfuscated (even compared to other `mutable std::unique_ptr` hacks).
This commit cleans up this situation and removes a totally superfluous
hidded 'modeling' sub-checker that did not have any role apart from
obstructing the normal initialization of bug types.
(Note that if we need to reintroduce CallAndMessageModeling in the
future, we can do it cleanly within the CheckerFamily framework, so we
wouldn't need to re-obfuscate the bug type initialization.)
This change is mostly non-functional, the only visible change is the
removal of the hidden modeling checker.
|
|
|
|
This reintroduces `Type.h`, having earlier been renamed to `TypeBase.h`,
as a redirection to `TypeBase.h`, and redirects most users to include
the former instead.
This is a preparatory patch for being able to provide inline definitions
for `Type` methods which would otherwise cause a circular dependency
with `Decl{,CXX}.h`.
Doing these operations into their own NFC patch helps the git rename
detection logic work, preserving the history.
This patch makes clang just a little slower to build (~0.17%), just
because it makes more code indirectly include `DeclCXX.h`.
|
|
This is a preparatory patch, to be able to provide inline definitions
for `Type` functions which depend on `Decl{,CXX}.h`. As the latter also
depends on `Type.h`, this would not be possible without some
reorganizing.
Splitting this rename into its own patch allows git to track this as a
rename, and preserve all git history, and not force any code
reformatting.
A later NFC patch will reintroduce `Type.h` as redirection to
`TypeBase.h`, rewriting most places back to directly including `Type.h`
instead of `TypeBase.h`, leaving only a handful of places where this is
necessary.
Then yet a later patch will exploit this by making more stuff inline.
|
|
|
|
TagDecls (#155463)
And make use of those.
These changes are split from prior PR #155028, in order to decrease the
size of that PR and facilitate review.
|
|
This changes a bunch of places which use getAs<TagType>, including
derived types, just to obtain the tag definition.
This is preparation for #155028, offloading all the changes that PR used
to introduce which don't depend on any new helpers.
|
|
cstring checker (#153498)
Prevent an assertion failure in the cstring checker when library
functions like memcpy are defined with non-default address spaces.
Adds a test for this case.
|
|
This patch replaces SmallSet<T *, N> with SmallPtrSet<T *, N>. Note
that SmallSet.h "redirects" SmallSet to SmallPtrSet for pointer
element types:
template <typename PointeeType, unsigned N>
class SmallSet<PointeeType*, N> : public SmallPtrSet<PointeeType*, N>
{};
We only have 30 instances that rely on this "redirection", with about
half of them under clang/. Since the redirection doesn't improve
readability, this patch replaces SmallSet with SmallPtrSet for pointer
element types.
I'm planning to remove the redirection eventually.
|
|
Addresses
https://github.com/llvm/llvm-project/pull/83027#discussion_r2264102109
We can only reach this part by a non-null `Ptr`, which also implies a
dereference of `PtrExpr`. Consequently, `PtrExpr` cannot be null here so
the check is pointless.
|
|
This is a major change on how we represent nested name qualifications in
the AST.
* The nested name specifier itself and how it's stored is changed. The
prefixes for types are handled within the type hierarchy, which makes
canonicalization for them super cheap, no memory allocation required.
Also translating a type into nested name specifier form becomes a no-op.
An identifier is stored as a DependentNameType. The nested name
specifier gains a lightweight handle class, to be used instead of
passing around pointers, which is similar to what is implemented for
TemplateName. There is still one free bit available, and this handle can
be used within a PointerUnion and PointerIntPair, which should keep
bit-packing aficionados happy.
* The ElaboratedType node is removed, all type nodes in which it could
previously apply to can now store the elaborated keyword and name
qualifier, tail allocating when present.
* TagTypes can now point to the exact declaration found when producing
these, as opposed to the previous situation of there only existing one
TagType per entity. This increases the amount of type sugar retained,
and can have several applications, for example in tracking module
ownership, and other tools which care about source file origins, such as
IWYU. These TagTypes are lazily allocated, in order to limit the
increase in AST size.
This patch offers a great performance benefit.
It greatly improves compilation time for
[stdexec](https://github.com/NVIDIA/stdexec). For one datapoint, for
`test_on2.cpp` in that project, which is the slowest compiling test,
this patch improves `-c` compilation time by about 7.2%, with the
`-fsyntax-only` improvement being at ~12%.
This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and
will reduce the performance impact of template specialization resugaring
when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the
reason is that I started by the nested name specifier part, before the
ElaboratedType part, but that had a huge performance downside, as
ElaboratedType is a big performance hog. I didn't have the steam to go
back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove
ElaboratedType in one go, versus removing it from one type at a time, as
that would present much more churn to the users. Also, the nested name
specifier having a different API avoids missing changes related to how
prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
`clang/include/clang/AST` and `clang/lib/AST`, with also important
changes in `clang/lib/Sema/TreeTransform.h`.
The rest and bulk of the changes are mostly consequences of the changes
in API.
PS: TagType::getDecl is renamed to `getOriginalDecl` in this patch, just
for easier to rebasing. I plan to rename it back after this lands.
Fixes #136624
Fixes https://github.com/llvm/llvm-project/issues/43179
Fixes https://github.com/llvm/llvm-project/issues/68670
Fixes https://github.com/llvm/llvm-project/issues/92757
|
|
This patch improves MallocChecker to detect use-after-free bugs when
a freed structure's field is passed by address (e.g., `&ptr->field`).
Previously, MallocChecker would miss such cases, as it only checked the
top-level symbol of argument values.
This patch analyzes the base region of arguments and extracts the
symbolic region (if any), allowing UAF detection even for field address
expressions.
Fixes #152446
|
|
Binding a value to location can happen when a new value is created or
when and existing value is updated. This modification exposes whether
the value binding happens at a declaration.
This helps simplify the hacky logic of the BindToImmutable checker.
|
|
This commit converts RetainCountChecker to the new checker family
framework that was introduced in the commit
6833076a5d9f5719539a24e900037da5a3979289
This commit also performs some minor cleanup around the parts that had
to be changed, but lots of technical debt still remains in this old
codebase.
|
|
CStringChecker had an AdditionOverflow bug type which was intended for a
situation where the analyzer concludes that the addition of two
size/length values overflows `size_t`.
I strongly suspect that the analyzer could emit bugs of this type in
certain complex corner cases (e.g. due to inaccurate cast modeling), but
these reports would be all false positives because in the real world the
sum of two size/length values is always far below SIZE_MAX. (Although
note that there was no test where the analyzer emitted a bug with this
type.)
To simplify the code (and perhaps eliminate false positives), I
eliminated this bug type and replaced code that emits it by a simple
`addSink()` call (because we still want to get rid of the execution
paths where the analyzer has invalid assumptions).
|
|
(#151908)
We sometimes don't know if the operand of [[assume]] is true/false, and
that's okay. We can just ignore the attribute in that case.
If we wanted something more fancy, we could bring the assumption to the
constraints, but dropping them should be just as fine for now.
Fixes #151854
|
|
This adds alpha.core.StoreToImmutable, a new alpha checker that detects
writes
to immutable memory regions, implementing part of SEI CERT Rule ENV30-C.
The
original proposal only handled global const variables, but this
implementation
extends it to also detect writes to:
- Local const variables
- String literals
- Const parameters and struct members
- Const arrays and pointers to const data
This checker is the continuation of the work started by zukatsinadze.
Discussion: https://reviews.llvm.org/D124244
|
|
This commit converts the class CStringChecker to the checker family
framework and slightly simplifies the implementation.
This commit is NFC and preserves the confused garbage descriptions and
categories of the bug types (which was inherited from old mistakes). I'm
planning to clean that up in a follow-up commit.
|
|
Fix false positive where warnings were asserted for placement new even
when no additional space is requested
The PlacementNewChecker incorrectly triggered warnings when the storage
provided matched or exceeded the allocated type size, causing false
positives. Now the warning triggers only when the provided storage is
strictly less than the required size.
Add test cases covering exact size, undersize, and oversize scenarios to
validate the fix.
Fixes #149240
|
|
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.
|
|
This commit converts the class `NSOrCFErrorDerefChecker` to the checker
family framework and simplifies some parts of the implementation (e.g.
removes two very trivial subclasses of `BugType`).
This commit is almost NFC, the only technically "functional" change is
that it removes the hidden modeling checker `NSOrCFErrorDerefChecker`
which was only relevant as an implementation detail of the old checker
registration procedure.
|
|
This commit eliminates some corrupted variants of the once-widespread
`mutable std::unique_ptr<BugType>` antipattern from the checker file
`BasicObjCFoundationChecks.cpp`.
Previous purges probably missed these because there are slight mutations
(e.g. using a subclass of `BugType` instead of `BugType`) and therefore
some natural search terms (e.g. `make_unique<BugType>`) didn't produce
matches in this file.
|
|
Changed the warning message:
- **From**: 'Attempt to free released memory'
**To**: 'Attempt to release already released memory'
- **From**: 'Attempt to free non-owned memory'
**To**: 'Attempt to release non-owned memory'
- **From**: 'Use of memory after it is freed'
**To**: 'Use of memory after it is released'
All connected tests and their expectations have been changed
accordingly.
Inspired by [this
PR](https://github.com/llvm/llvm-project/pull/147542#discussion_r2195197922)
|
|
This commit converts the class DereferenceChecker to the checker family
framework and simplifies some parts of the implementation.
This commit is almost NFC, the only technically "functional" change is
that it removes the hidden modeling checker `DereferenceModeling` which
was only relevant as an implementation detail of the old checker
registration procedure.
|
|
MallocChecker.cpp has a complex heuristic that supresses reports where
the memory release happens during the release of a reference-counted
object (to suppress a significant amount of false positives).
Previously this logic asserted that there is at most one release point
corresponding to a symbol, but it turns out that there is a rare corner
case where the symbol can be released, forgotten and then released
again. This commit removes that assertion to avoid the crash. (As this
issue just affects a bug suppression heuristic, I didn't want to dig
deeper and modify the way the state of the symbol is changed.)
Fixes #149754
|
|
sugar types (#149613)
The checks for the 'z' and 't' format specifiers added in the original
PR #143653 had some issues and were overly strict, causing some build
failures and were consequently reverted at
https://github.com/llvm/llvm-project/commit/4c85bf2fe8042c855c9dd5be4b02191e9d071ffd.
In the latest commit
https://github.com/llvm/llvm-project/pull/149613/commits/27c58629ec76a703fde9c0b99b170573170b4a7a,
I relaxed the checks for the 'z' and 't' format specifiers, so warnings
are now only issued when they are used with mismatched types.
The original intent of these checks was to diagnose code that assumes
the underlying type of `size_t` is `unsigned` or `unsigned long`, for
example:
```c
printf("%zu", 1ul); // Not portable, but not an error when size_t is unsigned long
```
However, it produced a significant number of false positives. This was
partly because Clang does not treat the `typedef` `size_t` and
`__size_t` as having a common "sugar" type, and partly because a large
amount of existing code either assumes `unsigned` (or `unsigned long`)
is `size_t`, or they define the equivalent of size_t in their own way
(such as
sanitizer_internal_defs.h).https://github.com/llvm/llvm-project/blob/2e67dcfdcd023df2f06e0823eeea23990ce41534/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h#L203
|
|
sugar types instead of built-in types (#143653)"
This reverts commit c27e283cfbca2bd22f34592430e98ee76ed60ad8.
A builbot failure has been reported:
https://lab.llvm.org/buildbot/#/builders/186/builds/10819/steps/10/logs/stdio
I'm also getting a large number of warnings related to %zu and %zx.
|
|
types instead of built-in types (#143653)
Including the results of `sizeof`, `sizeof...`, `__datasizeof`,
`__alignof`, `_Alignof`, `alignof`, `_Countof`, `size_t` literals, and
signed `size_t` literals, the results of pointer-pointer subtraction and
checks for standard library functions (and their calls).
The goal is to enable clang and downstream tools such as clangd and
clang-tidy to provide more portable hints and diagnostics.
The previous discussion can be found at #136542.
This PR implements this feature by introducing a new subtype of `Type`
called `PredefinedSugarType`, which was considered appropriate in
discussions. I tried to keep `PredefinedSugarType` simple enough yet not
limited to `size_t` and `ptrdiff_t` so that it can be used for other
purposes. `PredefinedSugarType` wraps a canonical `Type` and provides a
name, conceptually similar to a compiler internal `TypedefType` but
without depending on a `TypedefDecl` or a source file.
Additionally, checks for the `z` and `t` format specifiers in format
strings for `scanf` and `printf` were added. It will precisely match
expressions using `typedef`s or built-in expressions.
The affected tests indicates that it works very well.
Several code require that `SizeType` is canonical, so I kept `SizeType`
to its canonical form.
The failed tests in CI are allowed to fail. See the
[comment](https://github.com/llvm/llvm-project/pull/135386#issuecomment-3049426611)
in another PR #135386.
|
