https://github.com/cor3ntin updated https://github.com/llvm/llvm-project/pull/117914
>From d1e2a9ac8e329d0786f41a3ef4bedb61d7f7ffe5 Mon Sep 17 00:00:00 2001 From: Corentin Jabot <corentinja...@gmail.com> Date: Wed, 27 Nov 2024 20:03:29 +0100 Subject: [PATCH 1/2] [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@-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); >From 3239123f0dc89430ee0bd027f635dcf640f5d2b1 Mon Sep 17 00:00:00 2001 From: Corentin Jabot <corentinja...@gmail.com> Date: Wed, 27 Nov 2024 20:46:28 +0100 Subject: [PATCH 2/2] formatting --- clang/docs/ReleaseNotes.rst | 1 - clang/lib/Sema/SemaCast.cpp | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 20fad4354df3ee..70b0269d8cd759 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -596,7 +596,6 @@ Improvements to Clang's diagnostics - 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/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp index dc5abe43a4694b..e3159f968abc24 100644 --- a/clang/lib/Sema/SemaCast.cpp +++ b/clang/lib/Sema/SemaCast.cpp @@ -19,6 +19,7 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" #include "clang/AST/RecordLayout.h" +#include "clang/Basic/Diagnostic.h" #include "clang/Basic/PartialDiagnostic.h" #include "clang/Basic/TargetInfo.h" #include "clang/Lex/Preprocessor.h" @@ -1765,7 +1766,8 @@ TryStaticDowncast(Sema &Self, CanQualType SrcType, CanQualType DestType, << SrcType << DestType << OpRange << Self.LangOpts.RTTI; - if(Self.LangOpts.RTTI) + + if (Self.LangOpts.RTTI) D << FixItHint::CreateReplacement(OpRange.getBegin(), "dynamic_cast"); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
