Make sure code respects the GNU-extension `__attribute__((returns_nonnull))`.
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
This commit also reverts an old hack introduced by
49bd58f1ebe28d97e4949e9c757bc5dfd8b2d72f
because it is no longer needed
CPP-4741
...because it is too noisy to be useful right now, and its architecture
is terrible, so it can't act a starting point of future development.
The main problem with this checker is that it tries to do (or at least
fake) path-sensitive analysis without actually using the established
path-sensitive analysis engine.
Instead of actually tracking the symbolic values and the known
constraints on them, this checker blindly gropes the AST and uses
heuristics like "this variable was seen in a comparison operator
expression that is not a loop condition, so it's probably not too large"
(which was improved in a separate commit to at least ignore comparison
operators that appear after the actual `malloc()` call).
This might have been acceptable in 2011 (when this checker was added),
but since then we developed a significantly better standard approach for
analysis and this old relic doesn't deserve to remain in the codebase.
Needless to say, this primitive approach causes lots of false positives
(and presumably false negatives as well), which ensures that this alpha
checker won't be missed by the users.
Moreover, the goals of this checker would be questionable even if it had
a perfect implementation. It's very aggressive to assume that the
argument of malloc can overflow by default (unless the checker sees a
bounds check); and this produces too many false positives -- perhaps
even for an optin checker. It may be possible to eventually create a
useful (and properly path-sensitive) optin checker for these kinds of
suspicious code, but this is a very low priority goal.
Also note that we already have `alpha.security.TaintedAlloc` which
provides more practical heuristics for detecting somewhat similar
"argument of malloc may be too large" vulnerabilities.
At pointer subtraction only pointers are allowed that point into an
array (or one after the end), this fact was checker by the checker. This
check is now removed because it is a special case of array indexing
error that is handled by different checkers (like ArrayBoundsV2).
The checker could report false positives if pointer arithmetic was done
on pointers to non-array data before pointer subtraction. Another
problem is fixed that could cause false positive if members of the same
structure but in different memory objects are subtracted.
Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.
To fix this, let's just only add this no-alias property for the success
path, and that's it :)
Fixes#100901
Taint propagation is a a generic modeling feature of the Clang Static
Analyzer which many other checkers depend on. Therefore
GenericTaintChecker is split into a TaintPropagation modeling checker
and a GenericTaint reporting checker.
User documentation currently found at https://clang-analyzer.llvm.org is migrated to RST format.
This commit migrates all the relevant content, including suspicious or even clearly outdated parts. These issues will be cleaned up in separate follow-up commits (where the diff is not obscured by the format change). However, a few typos are fixed, and some parts (availability of binary releases, integration with XCode) are marked (with [Legacy] tags) to highlight that they are outdated.
The primary motivation for this change is to update the facts in the docs and make them discoverable from the RST-generated doc-tree as well (many subpages are not accessible at all, as the menu generation for the HTML-based page is not working at all).
This commit migrates all the relevant content, including parts that are
suspicious or even clearly outdated. These issues will be cleaned up in
separate follow-up commits (where the diff is not obscured by the format
change). However, a few typos are fixed and some parts (availability of
binary releases, integration with XCode) are marked (with [Legacy] tags)
to highlight that they are outdated.
The primary motivation for this change is to update the facts in the
docs and make them discoverable from the RST-generated doc-tree as well
(many subpages are not accessible **at all**, as the menu generation for
the HTML based page is not working at all).
The checker was renamed at some time ago but the documentation was not
updated. The section is now just moved and renamed. The documentation is
still very simple and needs improvement.
The checker `alpha.core.SizeofPtr` was a very simple checker that did
not rely on path sensitive analysis and was very similar to the (more
complex and refined) clang-tidy check `bugprone-sizeof-expression`.
As there is no reason to maintain two separate implementations for the
same goal (and clang-tidy is more lightweight and accessible than the
Analyzer) I decided to move this functionality from the Static Analyzer
to clang-tidy.
Recently my commit 546c816a529835a4cf89deecff957ea336a94fa2
reimplemented the advantageous parts of `alpha.core.SizeofPtr` within
clang-tidy; now this commit finishes the transfer by deleting
`alpha.core.SizeofPtr`.
A new optional checker (optin.taint.TaintedAlloc) will warn if a memory
allocation function (malloc, calloc, realloc, alloca, operator new[]) is
called with a tainted (attacker controlled) size parameter.
A large, maliciously set size value can trigger memory exhaustion. To
get this warning, the alpha.security.taint.TaintPropagation checker also
needs to be switched on.
The warning will only be emitted, if the analyzer cannot prove that the
size is below reasonable bounds (<SIZE_MAX/4).
Updated the documentation in `checkers.rst` to include an example of how
`trylock` function is handled.
Added a new test for a scenario where `pthread_mutex_trylock` is used,
demonstrating the current limitation.
The "cert" package looks not useful and the checker has not a meaningful name with the old naming scheme.
Additionally tests and documentation is updated.
Before this commit the the checker alpha.security.taint.TaintPropagation always reported warnings when the size argument of a memcpy-like or malloc-like function was tainted. However, this produced false positive reports in situations where the size was tainted, but correctly performed bound checks guaranteed the safety of the call.
This commit removes the rough "always warn if the size argument is tainted" heuristic; but it would be good to add a more refined "warns if the size argument is tainted and can be too large" heuristic in follow-up commits. That logic would belong to CStringChecker and MallocChecker, because those are the checkers responsible for the more detailed modeling of memcpy-like and malloc-like functions. To mark this plan, TODO comments are added in those two checkers.
There were several test cases that used these sinks to test generic properties of taint tracking; those were adapted to use different logic.
As a minor unrelated change, this commit ensures that strcat (and its wide variant, wcsncat) propagates taint from the first argument to the first argument, i.e. a tainted string remains tainted if we concatenate it with another string. This change was required because the adapted variant of multipleTaintedArgs is relying on strncat to compose a value that combines taint from two different sources.
The checker may create failure branches for all stream write operations
only if the new option "pedantic" is set to true.
Result of the write operations is often not checked in typical code. If
failure branches are created the checker will warn for unchecked write
operations and generate a lot of "false positives" (these are valid
warnings but the programmer does not care about this problem).
The checker finds a type of undefined behavior, where if the type of a
pointer to an object-array is different from the objects' underlying
type, calling `delete[]` is undefined, as the size of the two objects
might be different.
The checker has been in alpha for a while now, it is a simple checker
that causes no crashes, and considering the severity of the issue, it
has a low result-count on open-source projects (in my last test-run on
my usual projects, it had 0 results).
This commit cleans up the documentation and adds docs for the limitation
related to tracking through references, in addition to moving it to
`cplusplus`.
---------
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
Co-authored-by: whisperity <whisperity@gmail.com>
Default value of checker option `ModelPOSIX` is changed to `true`.
Documentation is updated.
This is a re-apply of commit 7af4e8bcc354d2bd7e46ecf547172b1f19ddde3e
that was reverted because a test failure (this is fixed now).
The checker reported a false positive on this code
void testTaintedSanitizedVLASize(void) {
int x;
scanf("%d", &x);
if (x<1)
return;
int vla[x]; // no-warning
}
After the fix, the checker only emits tainted warning if the vla size is
coming from a tainted source and it cannot prove that it is positive.
This checker does not exist (any more?) but appeared in the
documentation. No other references to CallAndMessageUnInitRefArg are
found in the full clang code.
This PR prepares the release notes of the Clang Static Analyzer for the
llvm-18 release branch, due in about a week.
See the regular [release schedule](https://llvm.org/docs/HowToReleaseLLVM.html#annual-release-schedule).
This patch was written after examining the relevant Static Analyzer
commits since the last release.
Have a look at the commits, and provide feedback if I missed anything
interesting.
Note that the release notes is not meant to be an exhaustive list of the
changes, but rather a curated list of the relevant changes that might
interest our stakeholders, such as tool vendors based on top of CSA or
users with custom checkers.
See the relevant commits by using this command:
```
git log --oneline llvmorg-18-init..llvm/main clang/{lib/StaticAnalyzer,include/clang/StaticAnalyzer} | grep -v NFC | grep -v -i revert
```
The checker EnumCastOutOfRange verifies the (helpful, but not
standard-mandated) design rule that integer to enum casts should not
produce values that don't have a corresponding enumerator. As it was
improved and cleaned up by recent changes, this commit renames it from
`alpha.cplusplus.EnumCastOutOfRange` to `optin.core.EnumCastOutOfRange`
to reflect that it's no longer alpha quality.
As this checker handles a basic language feature (which is also present
in plain C), I moved it to a "core" subpackage within "optin".
In addition to the renaming, this commit cleans up the documentation in
`checkers.rst` and adds the new example code to a test file to ensure
that it's indeed producing the behavior claimend in the documentation.
Thanks to recent improvements in #67663, InvalidPtr checker does not
emit any false positives on the following OS projects: memcached, tmux,
curl, twin, vim, openssl, sqlite, ffmpeg, postgres, tinyxml2, libwebm,
xerces, bitcoin, protobuf, qtbase, contour, acid, openrct2. (Before the
changes mentioned above, there were 27 reports, catching the `getenv`
invalidates previous `getenv` results cases. That strict behaviour is
disabled by default)
Introduce 'InvalidatingGetEnv' checker option for 'getenv' calls.
- POSIX suggests consecutive 'getenv' calls may invalidate
pointer pointers. This is often too strict in real-world scenarios.
- New 'InvalidatingGetEnv' checker option provides a more
pragmatic default that doesn't treat consecutive 'getenv'
calls as invalidating.
- Now also handles main function specifications with an
environment pointer as the third parameter.
Original Phabricator review:
https://reviews.llvm.org/D154603
This checker reports cases where an array of polymorphic objects are
deleted as their base class. Deleting an array where the array's static
type is different from its dynamic type is undefined.
Since the checker is similar to DeleteWithNonVirtualDtorChecker, I
refactored that checker to support more detection types.
This checker corresponds to the SEI Cert rule EXP51-CPP: Do not delete
an array through a pointer of the incorrect type.
Differential Revision: https://reviews.llvm.org/D158156
This commit releases a checker that was developed to a stable level in
the Ericsson-internal fork of Clang Static Analyzer.
Note that the functionality of this checker overlaps with
core.UndefinedBinaryOperatorResult ("UBOR"), but there are several
differences between them:
(1) UBOR is only triggered when the constant folding performed by the
Clang Static Analyzer engine determines that the value of a binary
operator expression is undefined; this checker can report issues where
the operands are not constants.
(2) UBOR has unrelated checks for handling other binary operators, this
checker only examines bitwise shifts.
(3) This checker has a Pedantic flag and by default does not report
expressions (e.g. -2 << 2) that're undefined by the standard but
consistently supported in practice.
(4) UBOR exhibits buggy behavior in code that involves cast expressions,
e.g.
void foo(unsigned short s) {
if (s == 2) {
(void) ((unsigned int) s) << 16;
}
}
Later it would be good to eliminate this overlap (perhaps by deprecating
and then eliminating the bitwise shift handling in UBOR), but in my
opinion that belongs to separate commits.
Differential Revision: https://reviews.llvm.org/D156312
Co-authored-by: Endre Fulop <endre.fulop@sigmatechnology.se>
The usage of the taint analysis is described through a command injection attack example.
It is explained how to make a variable sanitized through configuration.
Differential Revision: https://reviews.llvm.org/D145229
Main reason for this change is that these checkers were implemented in the same class
but had different dependency ordering. (NonNullParamChecker should run before StdCLibraryFunctionArgs
to get more special warning about null arguments, but the apiModeling.StdCLibraryFunctions was a modeling
checker that should run before other non-modeling checkers. The modeling checker changes state in a way
that makes it impossible to detect a null argument by NonNullParamChecker.)
To make it more simple, the modeling part is removed as separate checker and can be only used if
checker StdCLibraryFunctions is turned on, that produces the warnings too. Modeling the functions
without bug detection (for invalid argument) is not possible. The modeling of standard functions
does not happen by default from this change on.
Reviewed By: Szelethus
Differential Revision: https://reviews.llvm.org/D151225
