[clang] [Clang] Diagnose down casting static_cast (PR #117914)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 27 11:19:56 PST 2024
https://github.com/cor3ntin created https://github.com/llvm/llvm-project/pull/117914
Invalid down casts are a source of vulnerabilities. Its allowance in static_cast - which is otherwise
relatively safe - are recognized as dangerous by
various safety guidelines such as the core guidelines and misra.
While there exist a clang-tidy check, it seems reasonable, in the interest of safety to warn about that construct in clang and to propose its deprecation in WG21.
This is inspired by
[p3081r0](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3081r0.pdf)
which propose to promote static_cast to dynamic_cast silently, which would have further issues (suddently static_cast could produce a null pointer).
Part of the goal of this PR is therefore to demonstrate the viability of deprecating this construct.
>From d1e2a9ac8e329d0786f41a3ef4bedb61d7f7ffe5 Mon Sep 17 00:00:00 2001
From: Corentin Jabot <corentinjabot at gmail.com>
Date: Wed, 27 Nov 2024 20:03:29 +0100
Subject: [PATCH] [Clang] Diagnose down casting static_cast
Invalid down casts are a source of vulnerabilities.
Its allowance in static_cast - which is otherwise
relatively safe - are recognized as dangerous by
various safety guidelines such as the core guidelines and misra.
While there exist a clang-tidy check, it seems reasonable,
in the interest of safety to warn about that construct in clang
and to propose its deprecation in WG21.
This is inspired by
[p3081r0](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p3081r0.pdf)
which propose to promote static_cast to dynamic_cast silently, which
would have further issues (suddently static_cast could produce a null pointer).
Part of the goal of this PR is therefore to demonstrate the viability
of deprecating this construct.
---
clang/docs/ReleaseNotes.rst | 4 ++++
.../include/clang/Basic/DiagnosticSemaKinds.td | 3 +++
clang/lib/Sema/SemaCast.cpp | 10 ++++++++++
clang/test/Analysis/ArrayDelete.cpp | 17 ++++++++++-------
.../WebKit/call-args-safe-functions.cpp | 10 ++++++----
.../ref-cntbl-base-virtual-dtor-templates.cpp | 2 ++
clang/test/Analysis/cast-to-struct.cpp | 7 ++++---
clang/test/Analysis/ctor.mm | 12 ++++++------
clang/test/SemaCXX/address-space-conversion.cpp | 6 +++---
9 files changed, 48 insertions(+), 23 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b1b8f3bfa33b19..20fad4354df3ee 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -593,6 +593,10 @@ Improvements to Clang's diagnostics
- Clang now supports using alias templates in deduction guides, aligning with the C++ standard,
which treats alias templates as synonyms for their underlying types (#GH54909).
+- Added ``-Wstatic-downcast`` to diagnose potentially dangerous uses of ``static_cast`` that perform a base-to-derived cast
+ on pointers and references of polymorphic types.
+
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1d777f670097b5..2de5de50d86993 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7982,6 +7982,9 @@ def err_bad_reinterpret_cast_reference : Error<
def warn_undefined_reinterpret_cast : Warning<
"reinterpret_cast from %0 to %1 has undefined behavior">,
InGroup<UndefinedReinterpretCast>, DefaultIgnore;
+def warn_static_downcast : Warning<
+ "static downcast from %0 to %1 is potentially dangerous%select{|; did you mean 'dynamic_cast'?}2">,
+ InGroup<DiagGroup<"static-downcast">>;
// These messages don't adhere to the pattern.
// FIXME: Display the path somehow better.
diff --git a/clang/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp
index f98857f852b5af..dc5abe43a4694b 100644
--- a/clang/lib/Sema/SemaCast.cpp
+++ b/clang/lib/Sema/SemaCast.cpp
@@ -1759,6 +1759,16 @@ TryStaticDowncast(Sema &Self, CanQualType SrcType, CanQualType DestType,
Self.BuildBasePathArray(Paths, BasePath);
Kind = CK_BaseToDerived;
+
+ if (!CStyle && Self.LangOpts.CPlusPlus && SrcType->getAsCXXRecordDecl()->isPolymorphic()) {
+ auto D = Self.Diag(OpRange.getBegin(), diag::warn_static_downcast)
+ << SrcType << DestType
+ << OpRange
+ << Self.LangOpts.RTTI;
+ if(Self.LangOpts.RTTI)
+ D << FixItHint::CreateReplacement(OpRange.getBegin(), "dynamic_cast");
+ }
+
return TC_Success;
}
diff --git a/clang/test/Analysis/ArrayDelete.cpp b/clang/test/Analysis/ArrayDelete.cpp
index 6887e0a35fb8bd..94a17746414eb1 100644
--- a/clang/test/Analysis/ArrayDelete.cpp
+++ b/clang/test/Analysis/ArrayDelete.cpp
@@ -21,7 +21,7 @@ void sink(Base *b) {
}
void sink_cast(Base *b) {
- delete[] static_cast<Derived*>(b); // no-warning
+ delete[] static_cast<Derived*>(b); // expected-warning {{static downcast from 'Base' to 'Derived'}}
}
void sink_derived(Derived *d) {
@@ -62,7 +62,7 @@ void safe_function() {
delete[] d; // no-warning
Base *b = new Derived[10];
- delete[] static_cast<Derived*>(b); // no-warning
+ delete[] static_cast<Derived*>(b); // expected-warning {{static downcast from 'Base' to 'Derived'}}
Base *sb = new Derived[10];
sink_cast(sb); // no-warning
@@ -77,7 +77,8 @@ void multiple_derived() {
// expected-note at -1{{Deleting an array of 'DoubleDerived' objects as their base class 'Base' is undefined}}
Base *b2 = new DoubleDerived[10]; // expected-note{{Casting from 'DoubleDerived' to 'Base' here}}
- Derived *d2 = static_cast<Derived*>(b2); // expected-note{{Casting from 'Base' to 'Derived' here}}
+ Derived *d2 = static_cast<Derived*>(b2); // expected-note{{Casting from 'Base' to 'Derived' here}} \
+ // expected-warning {{static downcast from 'Base' to 'Derived'}}
delete[] d2; // expected-warning{{Deleting an array of 'DoubleDerived' objects as their base class 'Derived' is undefined}}
// expected-note at -1{{Deleting an array of 'DoubleDerived' objects as their base class 'Derived' is undefined}}
@@ -87,12 +88,13 @@ void multiple_derived() {
// expected-note at -1{{Deleting an array of 'DoubleDerived' objects as their base class 'Base' is undefined}}
Base *b4 = new DoubleDerived[10];
- Derived *d4 = static_cast<Derived*>(b4);
- DoubleDerived *dd4 = static_cast<DoubleDerived*>(d4);
+ Derived *d4 = static_cast<Derived*>(b4); // expected-warning {{static downcast from 'Base' to 'Derived'}}
+ DoubleDerived *dd4 = static_cast<DoubleDerived*>(d4); // expected-warning {{static downcast from 'Derived' to 'DoubleDerived'}}
delete[] dd4; // no-warning
Base *b5 = new DoubleDerived[10]; // expected-note{{Casting from 'DoubleDerived' to 'Base' here}}
- DoubleDerived *dd5 = static_cast<DoubleDerived*>(b5); // expected-note{{Casting from 'Base' to 'DoubleDerived' here}}
+ DoubleDerived *dd5 = static_cast<DoubleDerived*>(b5); // expected-note{{Casting from 'Base' to 'DoubleDerived' here}} \
+ // expected-warning {{static downcast from 'Base' to 'DoubleDerived'}}
Derived *d5 = dd5; // expected-note{{Casting from 'DoubleDerived' to 'Derived' here}}
delete[] d5; // expected-warning{{Deleting an array of 'DoubleDerived' objects as their base class 'Derived' is undefined}}
// expected-note at -1{{Deleting an array of 'DoubleDerived' objects as their base class 'Derived' is undefined}}
@@ -103,7 +105,8 @@ void unrelated_casts() {
Base &b2 = *b; // no-note: See the FIXME.
// FIXME: Displaying casts of reference types is not supported.
- Derived &d2 = static_cast<Derived&>(b2); // no-note: See the FIXME.
+ Derived &d2 = static_cast<Derived&>(b2); // expected-warning {{static downcast from 'Base' to 'Derived'}}
+ // no-note: See the FIXME.
Derived *d = &d2; // no-note: See the FIXME.
delete[] d; // expected-warning{{Deleting an array of 'DoubleDerived' objects as their base class 'Derived' is undefined}}
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-safe-functions.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-safe-functions.cpp
index a87446564870cd..322aa223db0570 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-safe-functions.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-safe-functions.cpp
@@ -1,5 +1,4 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
-// expected-no-diagnostics
class Base {
public:
@@ -30,18 +29,21 @@ template<typename Target, typename Source>
inline Target* dynamicDowncast(Source* source)
{
return static_cast<Target*>(source);
+ // expected-warning at -1 {{static downcast from 'Derived' to 'SubDerived'}}
}
template<typename Target, typename Source>
inline Target* checkedDowncast(Source* source)
{
return static_cast<Target*>(source);
+ // expected-warning at -1 {{static downcast from 'Derived' to 'SubDerived'}}
}
template<typename Target, typename Source>
inline Target* uncheckedDowncast(Source* source)
{
return static_cast<Target*>(source);
+ // expected-warning at -1 {{static downcast from 'Derived' to 'SubDerived'}}
}
template<typename... Types>
@@ -49,8 +51,8 @@ String toString(const Types&... values);
void foo(OtherObject* other)
{
- dynamicDowncast<SubDerived>(other->obj());
- checkedDowncast<SubDerived>(other->obj());
- uncheckedDowncast<SubDerived>(other->obj());
+ dynamicDowncast<SubDerived>(other->obj()); // expected-note {{in instantiation}}
+ checkedDowncast<SubDerived>(other->obj()); // expected-note {{in instantiation}}
+ uncheckedDowncast<SubDerived>(other->obj()); // expected-note {{in instantiation}}
toString(other->obj());
}
diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index 4fc1624d7a1544..c49e1e07ccfb5b 100644
--- a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -327,6 +327,7 @@ void UseDerivedClass11(DerivedClass12& obj) { obj.deref(); }
void deleteBase2(BaseClass2* obj) {
if (obj->isDerived())
delete static_cast<DerivedClass12*>(obj);
+ // expected-warning at -1 {{static downcast from 'BaseClass2' to 'DerivedClass12'}}
else
delete obj;
}
@@ -356,6 +357,7 @@ void UseDerivedClass11(DerivedClass13& obj) { obj.deref(); }
void BaseClass3::destory() {
if (isDerived())
delete static_cast<DerivedClass13*>(this);
+ // expected-warning at -1 {{static downcast from 'BaseClass3' to 'DerivedClass13'}}
else
delete this;
}
diff --git a/clang/test/Analysis/cast-to-struct.cpp b/clang/test/Analysis/cast-to-struct.cpp
index c3aba023c56e46..b994000aca7892 100644
--- a/clang/test/Analysis/cast-to-struct.cpp
+++ b/clang/test/Analysis/cast-to-struct.cpp
@@ -41,20 +41,21 @@ void structToStruct(struct AB *P) {
Base &B1 = D1;
D2 = (Derived *)&B1;
D2 = dynamic_cast<Derived *>(&B1);
- D2 = static_cast<Derived *>(&B1);
+ D2 = static_cast<Derived *>(&B1); // expected-warning {{static downcast from 'Base' to 'Derived'}}
// True positives when casting from Base to Derived.
Base B2;
D2 = (Derived *)&B2;// expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
D2 = dynamic_cast<Derived *>(&B2);// expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
- D2 = static_cast<Derived *>(&B2);// expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
+ D2 = static_cast<Derived *>(&B2);// expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}} \
+ // expected-warning {{static downcast from 'Base' to 'Derived'}}
// False negatives, cast from Base to Derived. With path sensitive analysis
// these false negatives could be fixed.
Base *B3 = &B2;
D2 = (Derived *)B3;
D2 = dynamic_cast<Derived *>(B3);
- D2 = static_cast<Derived *>(B3);
+ D2 = static_cast<Derived *>(B3); // expected-warning {{static downcast from 'Base' to 'Derived'}}
}
void intToStruct(int *P) {
diff --git a/clang/test/Analysis/ctor.mm b/clang/test/Analysis/ctor.mm
index 6ac9050fc29f70..2ab0aa59b1924e 100644
--- a/clang/test/Analysis/ctor.mm
+++ b/clang/test/Analysis/ctor.mm
@@ -849,11 +849,11 @@ void test() {
// constructor could have been called on an initialized piece of memory),
// so no uninitialized value warning here, and these should be symbols, not
// undefined values, for later comparison.
- glob_y = static_cast<D *>(this)->y;
- glob_z = static_cast<D *>(this)->z;
- glob_w = static_cast<D *>(this)->w;
- static_cast<D *>(this)->y = 2;
- static_cast<D *>(this)->z = 3;
- static_cast<D *>(this)->w = 4;
+ glob_y = static_cast<D *>(this)->y; // expected-warning {{static downcast}}
+ glob_z = static_cast<D *>(this)->z; // expected-warning {{static downcast}}
+ glob_w = static_cast<D *>(this)->w; // expected-warning {{static downcast}}
+ static_cast<D *>(this)->y = 2; // expected-warning {{static downcast}}
+ static_cast<D *>(this)->z = 3; // expected-warning {{static downcast}}
+ static_cast<D *>(this)->w = 4; // expected-warning {{static downcast}}
}
}
diff --git a/clang/test/SemaCXX/address-space-conversion.cpp b/clang/test/SemaCXX/address-space-conversion.cpp
index b1fb69816334df..3620d0997d5e8f 100644
--- a/clang/test/SemaCXX/address-space-conversion.cpp
+++ b/clang/test/SemaCXX/address-space-conversion.cpp
@@ -56,9 +56,9 @@ void test_static_cast(void_ptr vp, void_ptr_1 vp1, void_ptr_2 vp2,
(void)static_cast<A_ptr_2>(bp2);
// Well-formed downcast
- (void)static_cast<B_ptr>(ap);
- (void)static_cast<B_ptr_1>(ap1);
- (void)static_cast<B_ptr_2>(ap2);
+ (void)static_cast<B_ptr>(ap); // expected-warning {{static downcast from 'A' to 'B'}}
+ (void)static_cast<B_ptr_1>(ap1); // expected-warning {{static downcast}}
+ (void)static_cast<B_ptr_2>(ap2); // expected-warning {{static downcast}}
// Well-formed cast to/from void
(void)static_cast<void_ptr>(ap);
More information about the cfe-commits
mailing list