[Clang] Diagnose down casting static_cast #117914
Conversation
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: cor3ntin (cor3ntin) ChangesInvalid down casts are a source of vulnerabilities. Its allowance in static_cast - which is otherwise 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 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. Full diff: https://github.com/llvm/llvm-project/pull/117914.diff 9 Files Affected:
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@-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@-1{{Deleting an array of 'DoubleDerived' objects as their base class 'Derived' is undefined}}
@@ -87,12 +88,13 @@ void multiple_derived() {
// expected-note@-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@-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@-1 {{static downcast from 'Derived' to 'SubDerived'}}
}
template<typename Target, typename Source>
inline Target* checkedDowncast(Source* source)
{
return static_cast<Target*>(source);
+ // expected-warning@-1 {{static downcast from 'Derived' to 'SubDerived'}}
}
template<typename Target, typename Source>
inline Target* uncheckedDowncast(Source* source)
{
return static_cast<Target*>(source);
+ // expected-warning@-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@-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@-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);
|
|
If we don't feel like enabling this by default, a strategy would be to attach this warning to some |
You can test this locally with the following command:git-clang-format --diff bbbaeb5584b5f1ab38cc86a9e8ed64ec1dc926b6 3239123f0dc89430ee0bd027f635dcf640f5d2b1 --extensions cpp -- clang/lib/Sema/SemaCast.cpp clang/test/Analysis/ArrayDelete.cpp clang/test/Analysis/Checkers/WebKit/call-args-safe-functions.cpp clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp clang/test/Analysis/cast-to-struct.cpp clang/test/SemaCXX/address-space-conversion.cppView the diff from clang-format here.diff --git a/clang/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp
index e3159f968a..0917d52e54 100644
--- a/clang/lib/Sema/SemaCast.cpp
+++ b/clang/lib/Sema/SemaCast.cpp
@@ -1761,14 +1761,13 @@ TryStaticDowncast(Sema &Self, CanQualType SrcType, CanQualType DestType,
Self.BuildBasePathArray(Paths, BasePath);
Kind = CK_BaseToDerived;
- if (!CStyle && Self.LangOpts.CPlusPlus && SrcType->getAsCXXRecordDecl()->isPolymorphic()) {
+ if (!CStyle && Self.LangOpts.CPlusPlus &&
+ SrcType->getAsCXXRecordDecl()->isPolymorphic()) {
auto D = Self.Diag(OpRange.getBegin(), diag::warn_static_downcast)
- << SrcType << DestType
- << OpRange
- << Self.LangOpts.RTTI;
+ << SrcType << DestType << OpRange << Self.LangOpts.RTTI;
if (Self.LangOpts.RTTI)
- D << FixItHint::CreateReplacement(OpRange.getBegin(), "dynamic_cast");
+ D << FixItHint::CreateReplacement(OpRange.getBegin(), "dynamic_cast");
}
return TC_Success;
|
IMO, this is useful as on-by-default, but under its own flag. I haven't looked at the 'diff' yet, but that is my thoughts on it. |
clang/docs/ReleaseNotes.rst
Outdated
| - 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. | ||
|
|
||
|
|
There was a problem hiding this comment.
Should this be just 1 newline?
| auto D = Self.Diag(OpRange.getBegin(), diag::warn_static_downcast) | ||
| << SrcType << DestType | ||
| << OpRange | ||
| << Self.LangOpts.RTTI; |
There was a problem hiding this comment.
Is there value to some sort of 'note' where we point out where the inheritence happened? Though printing the whole inheritence list is probably pretty awful.
| "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">, |
There was a problem hiding this comment.
I think it is more than 'potentially dangerous', yes? Perhaps also we should mention the polymorphic-ness of the types as to why it is dangerous.
There was a problem hiding this comment.
if you cast to the correct derived class, it works!
Perhaps also we should mention the polymorphic-ness of the types as to why it is dangerous.
Yes
There was a problem hiding this comment.
Ah, right, yes... I wish we could explain that better in the brevity we have available here. I kinda hate this is a warning with false-positives, but IMO the goal of trying to make this a deprecated cast is worth the effort.
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Thanks for this! I think the direction is reasonable.
I'm a little bit worried about the behavior of the C++ Core Guideline check in clang-tidy though as it's now wholly redundant, isn't it? Perhaps that check should ensure this warning is enabled and otherwise do nothing? CC @HerrCai0907 @5chmidti @PiotrZSL
clang/lib/Sema/SemaCast.cpp
Outdated
| << SrcType << DestType | ||
| << OpRange | ||
| << Self.LangOpts.RTTI; | ||
| if(Self.LangOpts.RTTI) |
There was a problem hiding this comment.
| if(Self.LangOpts.RTTI) | |
| if (Self.LangOpts.RTTI) |
I think the checker can be removed if clang has this kind of compilation time warning and can totally cover the checker.
Then how to handle this downcast without RTTI support? |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Generally LGTM but we're missing test coverage for the ; did you mean 'dynamic_cast'? part of the diagnostic message. It'd be good to have a test with -frtti and -fno-rtti showing that we do the right thing there, including with the fix-it suggestion.
|
I think we need more discussion before enabling that by default tbh. as @HerrCai0907 mentioned people who do custom rtti or crtp sort of need to be able to use static_cast in some cases |
It only covers the whole check if
Afaik that would be a first. If there is an equivalent warning for a check, then the check can be removed I favor of the warning. In this case, I think that the check is probably going to stay, but that will be a question for the time when this PR is finalized/merged, as to how much the warning covers the check. @cor3ntin with CRTP there shouldn't be virtual functions, otherwise why use CRTP, which means the record fails the check for being polymorphic. |
6 participants
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
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.