| Message ID | 2cea52fd-a796-6022-2bfa-d968ce0d0c3f@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | handle bad __dynamic_cast more gracefully (PR 99074) | expand |
On 2/13/21 7:31 PM, Martin Sebor wrote: > The test case in PR 99074 invokes dynamic_cast with the this pointer > in a non-static member function called on a null pointer. The call > is, of course, undefined and other different circumstances would be > diagnosed by -Wnonnull. Unfortunately, in the test case, the null > pointer is the result of inlining and constant propagation and so > detected neither by the front end -Wnonnull nor by the middle end. > The program ends up passing it to __dynamic_cast() which then > crashes at runtime (again, not surprising for undefined behavior. > > However, the reporter says the code behaved gracefully (didn't crash) > when compiled with GCC 4.8, and in my tests it also doesn't crash > when compiled with Clang or ICC. I looked to see if it's possible > to do better and it seems it is. > > The attached patch improves things by changing __dynamic_cast to > fail by returning null when the first argument is null, and also This hunk is OK. > by declaring __dynamic_cast with attribute nonnull so that invalid > calls to it with a constant null pointer can be detected at compile > time. This is not; dynamic_cast is specified to return null for a null operand. "If v is a null pointer value, the result is a null pointer value." The undefined behavior is the call to _to_object, not the dynamic_cast. > Since the test case is undefined it seems borderline whether this > can strictly be considered a regression, even if some previous > releases handled it more gracefully. Indeed. But handling the null case in __dynamic_cast as well as in the compiler seems harmless enough. Jason
On 2/22/21 4:08 PM, Jason Merrill wrote: > On 2/13/21 7:31 PM, Martin Sebor wrote: >> The test case in PR 99074 invokes dynamic_cast with the this pointer >> in a non-static member function called on a null pointer. The call >> is, of course, undefined and other different circumstances would be >> diagnosed by -Wnonnull. Unfortunately, in the test case, the null >> pointer is the result of inlining and constant propagation and so >> detected neither by the front end -Wnonnull nor by the middle end. >> The program ends up passing it to __dynamic_cast() which then >> crashes at runtime (again, not surprising for undefined behavior. >> >> However, the reporter says the code behaved gracefully (didn't crash) >> when compiled with GCC 4.8, and in my tests it also doesn't crash >> when compiled with Clang or ICC. I looked to see if it's possible >> to do better and it seems it is. >> >> The attached patch improves things by changing __dynamic_cast to >> fail by returning null when the first argument is null, and also > > This hunk is OK. > >> by declaring __dynamic_cast with attribute nonnull so that invalid >> calls to it with a constant null pointer can be detected at compile >> time. > > This is not; dynamic_cast is specified to return null for a null operand. > > "If v is a null pointer value, the result is a null pointer value." > > The undefined behavior is the call to _to_object, not the dynamic_cast. Yes, of course. Just to be clear, in case it's not from the patch, it adds nonnull to the __dynamic_cast() function in libsupc++ which (if I read the comment right) documents nonnull-ness as its precondition. The function should never be called with a null pointer except in buggy code like in the PR. I don't think this is the most elegant way to diagnose the user bug but I also couldn't think of anything better. Do you have any suggestions? (I ask in part because for GCC 12 I'd like to see about issuing the warning requested on PR 12277.) WRT to the documentation of __dynamic_cast(), I didn't remove the comment in the patch that mentions the precondition because of the new warning. If we want to consider null pointers valid input to the function it seems we should update the comment. Do you agree? The comment is below. I assume SUB refers to SRC_PTR. /* sub: source address to be adjusted; nonnull, and since the * source object is polymorphic, *(void**)sub is a virtual pointer. * src: static type of the source object. * dst: destination type (the "T" in "dynamic_cast<T>(v)"). * src2dst_offset: a static hint about the location of the * source subobject with respect to the complete object; * special negative values are: * -1: no hint * -2: src is not a public base of dst * -3: src is a multiple public base type but never a * virtual base type * otherwise, the src type is a unique public nonvirtual * base type of dst at offset src2dst_offset from the * origin of dst. */ extern "C" void * __dynamic_cast (const void *src_ptr, // object started from const __class_type_info *src_type, // type of the starting object const __class_type_info *dst_type, // desired target type ptrdiff_t src2dst) // how src and dst are related > >> Since the test case is undefined it seems borderline whether this >> can strictly be considered a regression, even if some previous >> releases handled it more gracefully. > > Indeed. But handling the null case in __dynamic_cast as well as in the > compiler seems harmless enough. Okay. Martin > > Jason >
On 2/22/21 8:00 PM, Martin Sebor wrote: > On 2/22/21 4:08 PM, Jason Merrill wrote: >> On 2/13/21 7:31 PM, Martin Sebor wrote: >>> The test case in PR 99074 invokes dynamic_cast with the this pointer >>> in a non-static member function called on a null pointer. The call >>> is, of course, undefined and other different circumstances would be >>> diagnosed by -Wnonnull. Unfortunately, in the test case, the null >>> pointer is the result of inlining and constant propagation and so >>> detected neither by the front end -Wnonnull nor by the middle end. >>> The program ends up passing it to __dynamic_cast() which then >>> crashes at runtime (again, not surprising for undefined behavior. >>> >>> However, the reporter says the code behaved gracefully (didn't crash) >>> when compiled with GCC 4.8, and in my tests it also doesn't crash >>> when compiled with Clang or ICC. I looked to see if it's possible >>> to do better and it seems it is. >>> >>> The attached patch improves things by changing __dynamic_cast to >>> fail by returning null when the first argument is null, and also >> >> This hunk is OK. >> >>> by declaring __dynamic_cast with attribute nonnull so that invalid >>> calls to it with a constant null pointer can be detected at compile >>> time. >> >> This is not; dynamic_cast is specified to return null for a null operand. >> >> "If v is a null pointer value, the result is a null pointer value." >> >> The undefined behavior is the call to _to_object, not the dynamic_cast. > > Yes, of course. Just to be clear, in case it's not from the patch, > it adds nonnull to the __dynamic_cast() function in libsupc++ which > (if I read the comment right) documents nonnull-ness as its > precondition. The function should never be called with a null > pointer except in buggy code like in the PR. True. So the attribute is technically correct, but the resulting warning is misleading, since the user shouldn't need to know anything about the internal implementation of dynamic_cast, and the user-visible feature doesn't have that constraint. > I don't think this > is the most elegant way to diagnose the user bug but I also couldn't > think of anything better. Do you have any suggestions? (I ask in > part because for GCC 12 I'd like to see about issuing the warning > requested on PR 12277.) If we can manage to warn about a null argument to __dynamic_cast, I'd think we should also be able to warn about a null 'this' argument to _to_object. > WRT to the documentation of __dynamic_cast(), I didn't remove > the comment in the patch that mentions the precondition because > of the new warning. If we want to consider null pointers valid > input to the function it seems we should update the comment. Do > you agree? The comment is from the ABI (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#dynamic_cast-algorithm), we shouldn't change it. > The comment is below. I assume SUB refers to SRC_PTR. > > /* sub: source address to be adjusted; nonnull, and since the > * source object is polymorphic, *(void**)sub is a virtual pointer. > * src: static type of the source object. > * dst: destination type (the "T" in "dynamic_cast<T>(v)"). > * src2dst_offset: a static hint about the location of the > * source subobject with respect to the complete object; > * special negative values are: > * -1: no hint > * -2: src is not a public base of dst > * -3: src is a multiple public base type but never a > * virtual base type > * otherwise, the src type is a unique public nonvirtual > * base type of dst at offset src2dst_offset from the > * origin of dst. */ > extern "C" void * > __dynamic_cast (const void *src_ptr, // object started from > const __class_type_info *src_type, // type of the > starting object > const __class_type_info *dst_type, // desired target type > ptrdiff_t src2dst) // how src and dst are related >> >>> Since the test case is undefined it seems borderline whether this >>> can strictly be considered a regression, even if some previous >>> releases handled it more gracefully. >> >> Indeed. But handling the null case in __dynamic_cast as well as in >> the compiler seems harmless enough. > > Okay. I guess it's a question of which behavior is preferable in (this) case of undefined behavior. Doing something reasonable is in general a valid choice. OTOH, the crash revealed the undefined behavior in the testcase. Jason
On 2/22/21 7:03 PM, Jason Merrill wrote: > On 2/22/21 8:00 PM, Martin Sebor wrote: >> On 2/22/21 4:08 PM, Jason Merrill wrote: >>> On 2/13/21 7:31 PM, Martin Sebor wrote: >>>> The test case in PR 99074 invokes dynamic_cast with the this pointer >>>> in a non-static member function called on a null pointer. The call >>>> is, of course, undefined and other different circumstances would be >>>> diagnosed by -Wnonnull. Unfortunately, in the test case, the null >>>> pointer is the result of inlining and constant propagation and so >>>> detected neither by the front end -Wnonnull nor by the middle end. >>>> The program ends up passing it to __dynamic_cast() which then >>>> crashes at runtime (again, not surprising for undefined behavior. >>>> >>>> However, the reporter says the code behaved gracefully (didn't crash) >>>> when compiled with GCC 4.8, and in my tests it also doesn't crash >>>> when compiled with Clang or ICC. I looked to see if it's possible >>>> to do better and it seems it is. >>>> >>>> The attached patch improves things by changing __dynamic_cast to >>>> fail by returning null when the first argument is null, and also >>> >>> This hunk is OK. >>> >>>> by declaring __dynamic_cast with attribute nonnull so that invalid >>>> calls to it with a constant null pointer can be detected at compile >>>> time. >>> >>> This is not; dynamic_cast is specified to return null for a null >>> operand. >>> >>> "If v is a null pointer value, the result is a null pointer value." >>> >>> The undefined behavior is the call to _to_object, not the dynamic_cast. >> >> Yes, of course. Just to be clear, in case it's not from the patch, >> it adds nonnull to the __dynamic_cast() function in libsupc++ which >> (if I read the comment right) documents nonnull-ness as its >> precondition. The function should never be called with a null >> pointer except in buggy code like in the PR. > > True. So the attribute is technically correct, but the resulting > warning is misleading, since the user shouldn't need to know anything > about the internal implementation of dynamic_cast, and the user-visible > feature doesn't have that constraint. I agree. Let's revisit this in stage 1 and see if we can do better (see below). > >> I don't think this >> is the most elegant way to diagnose the user bug but I also couldn't >> think of anything better. Do you have any suggestions? (I ask in >> part because for GCC 12 I'd like to see about issuing the warning >> requested on PR 12277.) > > If we can manage to warn about a null argument to __dynamic_cast, I'd > think we should also be able to warn about a null 'this' argument to > _to_object. The -Wnonnull on the __dynamic_cast is readily detectable because the null constant is propagated into the call after it has been inlined into _to_object() by the time -Wnonnull runs (just after early inlining). The null this pointer isn't diagnosed for the same reason: the call to _to_object() is inlined before the null is fully propagated into the this argument. This can be fixed by enhancing the warning to look through use-def chains rather than just rely on constant propagation. But I also recently submitted a patch for PR 87489 to run -Wnonnull later, after FRE, to avoid a class of false positives and negatives. If that goes forward (in GCC 12) this solution won't work. > >> WRT to the documentation of __dynamic_cast(), I didn't remove >> the comment in the patch that mentions the precondition because >> of the new warning. If we want to consider null pointers valid >> input to the function it seems we should update the comment. Do >> you agree? > > The comment is from the ABI > (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#dynamic_cast-algorithm), > we shouldn't change it. Ack. > >> The comment is below. I assume SUB refers to SRC_PTR. >> >> /* sub: source address to be adjusted; nonnull, and since the >> * source object is polymorphic, *(void**)sub is a virtual pointer. >> * src: static type of the source object. >> * dst: destination type (the "T" in "dynamic_cast<T>(v)"). >> * src2dst_offset: a static hint about the location of the >> * source subobject with respect to the complete object; >> * special negative values are: >> * -1: no hint >> * -2: src is not a public base of dst >> * -3: src is a multiple public base type but never a >> * virtual base type >> * otherwise, the src type is a unique public nonvirtual >> * base type of dst at offset src2dst_offset from the >> * origin of dst. */ >> extern "C" void * >> __dynamic_cast (const void *src_ptr, // object started from >> const __class_type_info *src_type, // type of the >> starting object >> const __class_type_info *dst_type, // desired target >> type >> ptrdiff_t src2dst) // how src and dst are related >>> >>>> Since the test case is undefined it seems borderline whether this >>>> can strictly be considered a regression, even if some previous >>>> releases handled it more gracefully. >>> >>> Indeed. But handling the null case in __dynamic_cast as well as in >>> the compiler seems harmless enough. >> >> Okay. > > I guess it's a question of which behavior is preferable in (this) case > of undefined behavior. Doing something reasonable is in general a valid > choice. OTOH, the crash revealed the undefined behavior in the testcase. I've committed the libsupc++ subset in r11-7350. Martin
PR c++/99074 - crash in dynamic_cast<>() on null pointer gcc/cp/ChangeLog: PR c++/99074 * rtti.c (build_dynamic_cast_1): Declare nonnull. libstdc++-v3/ChangeLog: PR c++/99074 * libsupc++/dyncast.cc (__dynamic_cast): Return null when first argument is null. gcc/testsuite/ChangeLog: PR c++/99074 * g++.dg/warn/Wnonnull11.C: New test. diff --git a/gcc/cp/rtti.c b/gcc/cp/rtti.c index b41d95469c6..b78b5f2dab4 100644 --- a/gcc/cp/rtti.c +++ b/gcc/cp/rtti.c @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see #include "stor-layout.h" #include "c-family/c-pragma.h" #include "gcc-rich-location.h" +#include "attribs.h" /* C++ returns type information to the user in struct type_info objects. We also use type information to implement dynamic_cast and @@ -767,6 +768,11 @@ build_dynamic_cast_1 (location_t loc, tree type, tree expr, dcast_fn = (build_library_fn_ptr (fn_name, fn_type, ECF_LEAF | ECF_PURE | ECF_NOTHROW)); pop_abi_namespace (flags); + + /* __dynamic_cast expects all nonnull pointers. */ + tree attr = tree_cons (get_identifier ("nonnull"), + NULL_TREE, NULL_TREE); + decl_attributes (&dcast_fn, attr, 0); dynamic_cast_node = dcast_fn; } result = build_cxx_call (dcast_fn, 4, elems, complain); diff --git a/gcc/testsuite/g++.dg/warn/Wnonnull11.C b/gcc/testsuite/g++.dg/warn/Wnonnull11.C new file mode 100644 index 00000000000..1dcd820356a --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wnonnull11.C @@ -0,0 +1,40 @@ +/* PR c++/99074 - gcc 8 and above is crashing with dynamic_cast<>() on null + pointer with optimization level -O1 and above + { dg-do run } + { dg-options "-O1 -Wall" } */ + +class Base +{ +public: + virtual ~Base() {} + virtual void op() = 0; +}; + +class Object: public virtual Base { }; + +class AbstractBase: public virtual Base +{ +public: + Object* _to_object () + { + return dynamic_cast<Object*>(this); // { dg-warning "\\\[-Wnonnull" } + } +}; + +class MyAbstractClass: public virtual AbstractBase +{ +public: + static MyAbstractClass* _nil () { return 0; } +}; + + +int main () +{ + MyAbstractClass *my_abs_type = MyAbstractClass::_nil (); + AbstractBase *abs_base = my_abs_type; + Object *obj = abs_base->_to_object (); + + __builtin_printf ("object is: %p\n", obj); + + return 0; +} diff --git a/libstdc++-v3/libsupc++/dyncast.cc b/libstdc++-v3/libsupc++/dyncast.cc index b7d98495ad3..f8f707ee4d4 100644 --- a/libstdc++-v3/libsupc++/dyncast.cc +++ b/libstdc++-v3/libsupc++/dyncast.cc @@ -47,6 +47,9 @@ __dynamic_cast (const void *src_ptr, // object started from const __class_type_info *dst_type, // desired target type ptrdiff_t src2dst) // how src and dst are related { + if (!src_ptr) + /* Handle precondition violations gracefully. */ + return NULL; const void *vtable = *static_cast <const void *const *> (src_ptr); const vtable_prefix *prefix = (adjust_pointer <vtable_prefix>