Array subscript on a const size array is not bounds-checked. The idiomatic
replacement is std::array which is bounds-safe in hardened mode of libc++.
This commit extends the fixit-producing machine to consider std::array as a
transformation target type and teaches it to handle the array subscript on const
size arrays with a trivial (empty) fixit.
Constructing `std::span` objects with the two parameter constructors
could introduce mismatched bounds information, which defeats the
purpose of using `std::span`. Therefore, we warn every use of such
constructors.
rdar://115817781
The patch fixes the crash introduced by the DataInvocation warning
gadget designed to warn against unsafe invocations of span::data method.
It also now considers the invocation of span::data method inside
parenthesis.
Radar: 121223051
---------
Co-authored-by: MalavikaSamak <malavika2@apple.com>
…-Wunsafe-buffer-usage,
there maybe accidental re-introduction of new OutOfBound accesses into
the code bases. One such case is invoking span::data() method on a span
variable to retrieve a pointer, which is then cast to a larger type and
dereferenced. Such dereferences can introduce OutOfBound accesses.
To address this, a new WarningGadget is being introduced to warn against
such invocations.
---------
Co-authored-by: MalavikaSamak <malavika2@apple.com>
…… (#68394)"
The new warnings are now under a separate flag
`-Wthread-safety-reference-return`, which is on by default under
`-Wthread-safety-reference`.
- People can opt out via `-Wthread-safety-reference
-Wnothread-safety-reference-return`.
This reverts commit 859f2d032386632562521a99db20923217d98988.
… (#67776)"
This detects issues in `scudo`. Reverting until these are fixed.
```
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:74:12: error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' exclusively [-Werror,-Wthread-safety-reference]
74 | return QuarantineCache;
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:248:28: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::getQuarantineCache' requested here
248 | Quarantine.drain(&TSD->getQuarantineCache(),
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:57:15: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::commitBack' requested here
57 | Instance->commitBack(this);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172:27: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::commitBack' requested here
172 | TSDRegistryT::ThreadTSD.commitBack(Instance);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:33:46: note: in instantiation of function template specialization 'scudo::teardownThread<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>' requested here
33 | CHECK_EQ(pthread_key_create(&PThreadKey, teardownThread<Allocator>), 0);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:42:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::init' requested here
42 | init(Instance); // Sets Initialized.
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:130:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initOnceMaybe' requested here
130 | initOnceMaybe(Instance);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:74:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThread' requested here
74 | initThread(Instance, MinimalInit);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:221:17: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThreadMaybe' requested here
221 | TSDRegistry.initThreadMaybe(this, MinimalInit);
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:790:5: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::initThreadMaybe' requested here
790 | initThreadMaybe();
| ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/wrappers_c.inc:36:25: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::canReturnNull' requested here
36 | if (SCUDO_ALLOCATOR.canReturnNull()) {
```
This reverts commit 6dd96d6e80e9b3679a6161c590c60e0e99549b89.
...of guarded variables, when the function is not marked as requiring
locks:
```
class Return {
Mutex mu;
Foo foo GUARDED_BY(mu);
Foo &returns_ref_locked() {
MutexLock lock(&mu);
return foo; // BAD
}
Foo &returns_ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo; // OK
}
};
```
Review on Phabricator: https://reviews.llvm.org/D153131
For a function `F` whose parameters need to be fixed, we group fix-its
of F's parameters together so that either all of the parameters get
fixed or none of them gets fixed.
Reviewed by: NoQ (Artem Dergachev), t-rasmud (Rashmi Mudduluru), jkorous (Jan Korous)
Differential revision: https://reviews.llvm.org/D153059
This reverts commit ac9a76d7487b9af1ace626eb90064194cb12c53d.
Previously an abstract class has no pure virtual function. It causes build error on some bots.
Slightly refactor and optimize the code in preparation for
implementing grouping parameters for a single fix-it.
Reviewed by: NoQ (Artem Dergachev), t-rasmud (Rashmi Mudduluru)
Differential revision: https://reviews.llvm.org/D156474
This patch introduces a new warning flag -Wtautological-negation-compare grouped in -Wtautological-compare that warns on the use of && or || operators against a variable and its negation.
e.g. x || !x and !x && x
This also makes the -Winfinite-recursion diagnose more cases.
Fixes https://github.com/llvm/llvm-project/issues/56035
Differential Revision: https://reviews.llvm.org/D152093
This does the rename for most internal uses of C2x, but does not rename
or reword diagnostics (those will be done in a follow-up).
I also updated standards references and citations to the final wording
in the standard.
For a fix-it that inserts the `[[clang::unsafe_buffer_usage]]`
attribute, it will lookup existing macros defined for the attribute
and use the (last defined such) macro directly. Fix-its will use raw
`[[clang::unsafe_buffer_usage]]` if no such macro is defined.
The implementation mimics how a similar machine for the
`[[fallthrough]]` attribute was implemented.
Reviewed by: NoQ (Artem Dergachev)
Differential revision: https://reviews.llvm.org/D150338
This patch implements a new clang driver flag -fsafe-buffer-usage-suggestions
which allows turning the smart suggestion machine on and off (defaults to off).
This is valuable for stability reasons, as the machine is being rapidly improved\
and we don't want accidental breakages to ruin the build for innocent users.
It is also arguably useful in general because it enables separation of concerns
between project contributors: some users will actively update the code to
conform to the programming model, while others simply want to make sure that
they aren't regressing it. Finally, there could be other valid reasons to
opt out of suggestions entirely on some codebases (while continuing to enforce
-Wunsafe-buffer-usage warnings), such as lack of access to hardened libc++
(or even to the C++ standard library in general) on the target platform.
When the flag is disabled, the unsafe buffer usage analysis is reduced to
an extremely minimal mode of operation that contains virtually no smarts:
not only it doesn't offer automatic fixits, but also textual suggestions
such as "change the type of this variable to std::span to preserve bounds
information" are not displayed, and in fact the machine doesn't even try
to blame specific variables in the first place, it simply warns on
the operations and leaves everything else to the user. So this flag turns off
a lot more of our complex machinery than what we already turn off in presence
of say -fno-diagnostic-fixit-info.
The flag is discoverable: when it's off, the warnings are accompanied by a note:
telling the user that there's a flag they can use.
Differential Revision: https://reviews.llvm.org/D146669
A follow-up change for 6d861d498de1320d22771c329ec69f9419ef06b7:
remove an unnecessary const-qualifier so that the code doesn't have to
remove the qualifier explicitly using `std::remove_const_t`, which
triggers a warning at some bots (e.g.,
https://lab.llvm.org/buildbot/#/builders/247/builds/4442).
The unsafe-buffer analysis requires a complete view of the translation
unit (TU) to be conservative. So the analysis is moved to the end of a
TU.
A summary of changes made: add a new `IssueWarnings` function in
`AnalysisBasedWarnings.cpp` for TU-based analyses. So far
[-Wunsafe-buffer-usage] is the only analysis using it but there could
be more. `Sema` will call the new `IssueWarnings` function at the end
of parsing a TU.
Reviewed by: NoQ (Artem Dergachev)
Differential revision: https://reviews.llvm.org/D146342
This patch checks whether -Wunreachable-code-fallthrough is enabled
when clang encounters unreachable fallthrough attributes and, if so,
suppresses code will never be executed warning to avoid duplicate
warnings.
Fixes https://github.com/llvm/llvm-project/issues/60416
Differential Revision: https://reviews.llvm.org/D145842
All exceptions thrown in coroutine bodies are caught and
unhandled_exception member of the coroutine promise type is called.
In accordance with the existing rules of diagnostics related to
exceptions thrown in functions marked noexcept, even if the promise
type's constructor, get_return_object, or unhandled_exception
throws, diagnostics should not be emitted.
Fixes#48797.
Differential Revision: https://reviews.llvm.org/D144352
The transformation strategy we are bringing up heavily relies on std::span which was introduced as part of C++20.
Differential Revision: https://reviews.llvm.org/D143455
Add a pair of clang pragmas:
- `#pragma clang unsafe_buffer_usage begin` and
- `#pragma clang unsafe_buffer_usage end`,
which specify the start and end of an (unsafe buffer checking) opt-out
region, respectively.
Behaviors of opt-out regions conform to the following rules:
- No nested nor overlapped opt-out regions are allowed. One cannot
start an opt-out region with `... unsafe_buffer_usage begin` but never
close it with `... unsafe_buffer_usage end`. Mis-use of the pragmas
will be warned.
- Warnings raised from unsafe buffer operations inside such an opt-out
region will always be suppressed. This behavior CANNOT be changed by
`clang diagnostic` pragmas or command-line flags.
- Warnings raised from unsafe operations outside of such opt-out
regions may be reported on declarations inside opt-out
regions. These warnings are NOT suppressed.
- An un-suppressed unsafe operation warning may be attached with
notes. These notes are NOT suppressed as well regardless of whether
they are in opt-out regions.
The implementation maintains a separate sequence of location pairs
representing opt-out regions in `Preprocessor`. The `UnsafeBufferUsage`
analyzer reads the region sequence to check if an unsafe operation is
in an opt-out region. If it is, discard the warning raised from the
operation immediately.
This is a re-land after I reverting it at 9aa00c8a306561c4e3ddb09058e66bae322a0769.
The compilation error should be resolved.
Reviewed by: NoQ
Differential revision: https://reviews.llvm.org/D140179
Add a pair of clang pragmas:
- `#pragma clang unsafe_buffer_usage begin` and
- `#pragma clang unsafe_buffer_usage end`,
which specify the start and end of an (unsafe buffer checking) opt-out
region, respectively.
Behaviors of opt-out regions conform to the following rules:
- No nested nor overlapped opt-out regions are allowed. One cannot
start an opt-out region with `... unsafe_buffer_usage begin` but never
close it with `... unsafe_buffer_usage end`. Mis-use of the pragmas
will be warned.
- Warnings raised from unsafe buffer operations inside such an opt-out
region will always be suppressed. This behavior CANNOT be changed by
`clang diagnostic` pragmas or command-line flags.
- Warnings raised from unsafe operations outside of such opt-out
regions may be reported on declarations inside opt-out
regions. These warnings are NOT suppressed.
- An un-suppressed unsafe operation warning may be attached with
notes. These notes are NOT suppressed as well regardless of whether
they are in opt-out regions.
The implementation maintains a separate sequence of location pairs
representing opt-out regions in `Preprocessor`. The `UnsafeBufferUsage`
analyzer reads the region sequence to check if an unsafe operation is
in an opt-out region. If it is, discard the warning raised from the
operation immediately.
Reviewed by: NoQ
Differential revision: https://reviews.llvm.org/D140179
Use clang fix-its to transform declarations of local variables, which
are used for buffer access , to be of std::span type.
We placed a few limitations to keep the solution simple:
- it only transforms local variable declarations (no parameter declaration);
- it only considers single level pointers, i.e., pointers of type T * regardless of whether T is again a pointer;
- it only transforms to std::span types (no std::array, or std::span::iterator, or ...);
- it can only transform a VarDecl that belongs to a DeclStmt whose has a single child.
One of the purposes of keeping this patch simple enough is to first
evaluate if fix-it is an appropriate approach to do the
transformation.
This commit was reverted by 622be09c815266632e204eaf1c7a35f050220459
for a compilation warning and now it is fixed.
Reviewed by: NoQ, jkorous
Differential revision: https://reviews.llvm.org/D139737
Use clang fix-its to transform declarations of local variables, which are used for buffer access , to be of std::span type.
We placed a few limitations to keep the solution simple:
- it only transforms local variable declarations (no parameter declaration);
- it only considers single level pointers, i.e., pointers of type T * regardless of whether T is again a pointer;
- it only transforms to std::span types (no std::array, or std::span::iterator, or ...);
- it can only transform a VarDecl that belongs to a DeclStmt whose has a single child.
One of the purposes of keeping this patch simple enough is to first
evaluate if fix-it is an appropriate approach to do the
transformation.
Reviewed by: NoQ, jkorous
Differential revision: https://reviews.llvm.org/D139737
This way we highlight a particular unsafe subexpression by providing more accurate
source location than begin of an entire statement.
Differential Revision: https://reviews.llvm.org/D141340
This patch adds more abstractions that we'll need later for emitting
-Wunsafe-buffer-usage fixits. It doesn't emit any actual fixits,
so no change is observed behavior, but it introduces a way to emit fixits,
and existing tests now verify that the compiler still emits no fixits,
despite knowing how to do so.
The purpose of our code transformation analysis is to fix variable types
in the code from raw pointer types to C++ standard collection/view types.
The analysis has to decide on its own which specific type is
the most appropriate for every variable. This patch introduces
the Strategy class that maps variables to their most appropriate types.
In D137348 we've introduced the Gadget abstraction, which describes
a rigid AST pattern that the analysis "fully understands" and may need
to fix. Which specific fix is actually necessary for a given Gadget,
and whether it's necessary at all, and whether it's possible in the first place,
depends on the Strategy. So, this patch adds a virtual method which every
gadget can implement in order to teach the analysis how to fix that gadget:
Gadget->getFixits(Strategy)
However, even if the analysis knows how to fix every Gadget, doesn't
necessarily mean it can fix the variable. Some uses of the variable may have
never been covered by Gadgets, which corresponds to the situation that
the analysis doesn't fully understand how the variable is used. This patch
introduces a Tracker class that tracks all variable uses (i.e. DeclRefExprs)
in the function. Additionally, each Gadget now provides a new virtual method
Gadget->getClaimedVarUseSites()
that the Tracker can call to see which DeclRefExprs are "claimed" by the Gadget.
In order to fix the variable with a certain Strategy, the Tracker needs to
confirm that there are no unclaimed uses, and every Gadget has to provide
a fix for that Strategy.
This "conservative" behavior guarantees that fixes emitted by our analysis
are correct by construction. We can now be sure that the analysis won't
attempt to emit a fix if it doesn't understand the code. Later, as we implement
more getFixits() methods in individual Gadget classes, we'll start
progressively emitting more and more fixits.
Differential Revision: https://reviews.llvm.org/D138253
