Introduce `-Wthread-safety-pointer` to warn when passing or returning
pointers to guarded variables or guarded data. This is is analogous to
`-Wthread-safety-reference`, which performs similar checks for C++
references.
Adding checks for pointer passing is required to avoid false negatives
in large C codebases, where data structures are typically implemented
through helpers that take pointers to instances of a data structure.
The feature is planned to be enabled by default under `-Wthread-safety`
in the next release cycle. This gives time for early adopters to address
new findings.
Pull Request: https://github.com/llvm/llvm-project/pull/127396
Unsafe operation in methods that are already annotated with
clang::unsafe_buffer_usage attribute, should not trigger a warning. This
is because, the developer has already identified the method as unsafe
and warning at every unsafe operation is redundant.
rdar://138644831
---------
Co-authored-by: MalavikaSamak <malavika2@apple.com>
This merges several falloff and noreturn-related warnings and
removes unused diagnostic arguments.
Changes:
- `warn_maybe_falloff_nonvoid_function` and
`warn_falloff_nonvoid_function`, `warn_maybe_falloff_nonvoid_coroutine`
and `warn_falloff_nonvoid_coroutine`,
`warn_maybe_falloff_nonvoid_lambda` and `warn_falloff_nonvoid_lambda`
were combined into `warn_falloff_nonvoid`,
- `err_maybe_falloff_nonvoid_block` and `err_falloff_nonvoid_block` were
combined into `err_falloff_nonvoid`
- `err_noreturn_block_has_return_expr` and
`err_noreturn_lambda_has_return_expr` were merged into
`err_noreturn_has_return_expr` with the same semantics as
`warn_falloff_nonvoid` or `err_falloff_nonvoid`.
- Removed some diagnostic args that weren’t being used by the diagnostics.
Declaring a coroutine `[[noreturn]]` doesn't make sense, because it will
always return its handle. Clang previously crashed when trying to warn
about this (diagnostic ID was 0).
Fixes#127327.
This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
thread safety, extending the scope of analysis to track these across
function boundries.
- Verify that the underlying mutexes of function arguments match the
expectations set by the annotations.
Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
This commit addresses several Static Analyzer issues related to
potential null dereference by replacing dyn_cast<> with cast<> and
getAs<> with castAs<> in various parts of the codes.
The cast function asserts that the cast is valid, ensuring that the
pointer is not null and preventing null dereference errors.
The changes are made in the following files:
CGBuiltin.cpp: Ensure vector types have exactly 3 elements.
CGExpr.cpp: Ensure member declarations are field declarations.
AnalysisBasedWarnings.cpp: Ensure operations are member expressions.
SemaExprMember.cpp: Ensure base types are extended vector types.
These changes ensure that the types are correctly cast and prevent
potential null dereference issues, improving the robustness and safety
of the code.
Fixed the crash coming from attempting to get size of incomplete types.
Casting `span.data()` to a pointer-to-incomplete-type should be
immediately considered unsafe.
Solving issue #116286.
Co-authored-by: Ziqing Luo <ziqing_luo@apple.com>
Emit a warning when the raw pointer retrieved from std::vector and
std::array instances are cast to a larger type. Such a cast followed by
a field dereference to the resulting pointer could cause an OOB access.
This is similar to the existing span::data warning.
(rdar://136704278)
Co-authored-by: MalavikaSamak <malavika2@apple.com>
Revert commit 23457964392d00fc872fa6021763859024fb38da, and re-land
with a new flag "-Wunsafe-buffer-usage-in-libc-call" for the new
warning.
(rdar://117182250)
[-Wunsafe-buffer-usage] Add warn on unsafe calls to libc functions
Warning about calls to libc functions involving buffer access. Warned
functions are hardcoded by names.
(rdar://117182250)
Extend the unsafe_buffer_usage attribute, so they can also be added to
struct fields. This will cause the compiler to warn about the unsafe
field at their access sites.
Co-authored-by: MalavikaSamak <malavika2@apple.com>
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling a
CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size), but
that is no longer the case. We want the Reporter to generate different
warnings (in the -Wunsafe-buffer-usage-in-container subgroup) for the
span contructor. And we will want it to report more warnings for other
std-container-specific gadgets in the future. To handle this we allow
the gadget to control if the warning is general (it calls
handleUnsafeBufferUsage()) or is a std-container-specific warning (it
calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
Issue #80482
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
