Repository navigation
_PyObject_CAST breaks code which assumed a static/dynamic casting #94731
Description
Activity
The code is in
_Py_CAST_impl:template <typename type, typename expr_type> inline type _Py_CAST_impl(expr_type *expr) { return reinterpret_cast<type>(expr); }AFAICS, C-style cast, with its special-snowflake semantics, is the only kind of cast we can switch to:
static_cast<>,const_cast<>is not enough heredynamic_cast<>won't work as we can't assume C++ inheritancereinterpret_cast<>breaks cases that do use C++ inheritance
And since avoiding C-style casts (
-Wold-style-cast) was the only reason for the elaborate C++-specific code of_Py_CAST_impl, if we need to use a C-style cast we might as well use the C definition for_Py_CAST.@mhammond, do you happen to have a complete reproducer?
If you have the time to construct it, I'd appreciate it. I'll try, but I have neither much C++ practice, nor a Windows box.
You don't actually need a cast here at all, because
MyObjectis aPyObjectin C++ so converts directly without a cast.You could do it easily in C++11 with
std::enable_ifandstd::is_convertiblebut you'd need to implement it yourself on versions before that. Fairly sure it's do-able - the challenge is doing it without too creating too many helpersYou don't actually need a cast here at all, because
MyObjectis aPyObjectin C++ so converts directly without a cast.Right, which is why the example above is not doing a cast. The
_Py_CASTin the Python headers are though.A C++11-only reproducer would help as well.
I believe the following should work:
template <typename type, typename expr_type> type _Py_REINTERPRET_CAST_if_needed(type expr, type dummy=static_cast<expr_type*>(NULL)) { return expr; } template <typename type, typename expr_type> type _Py_REINTERPRET_CAST_if_needed(expr_type *expr, ...) { return reinterpret_cast<type>(expr); } template <typename type, typename expr_type> inline type _Py_CAST_impl(expr_type *expr) { return _Py_REINTERPRET_CAST_if_needed<type, expr_type>(expr, NULL); }Essentially the first version of
_Py_REINTERPRET_CAST_if_neededis only available ifexpr_type*can be converted totypedirectly. If it's available it's preferred over the second version of_Py_REINTERPRET_CAST_if_needed, otherwise the second version is used.
However, this whole mechanism seems to miss the point of using the C++ casts - the reason they're preferred over C casts is because you've narrowed down the list of operations they can perform (so have kind of checked your assumptions). When you create elaborate mechanisms to call whatever type of cast is required, there''s really no advantage over using a C cast, so why bother?
I can submit a PR for the overly elaborate but working mechanism if useful. Should be easy enough to create a test.
Other comments imply this mechanism is to enable capabilities inside Python itself - maybe another option is to work out how to make a more formal distinction between external code which is already doing the right thing vs these internal requirements?
AFAIK, the reason the elaborate mechanism was added was to avoid "old-style cast" warnings.
IMO it'd be best to revert to using C-style casts in 3.11 (so behavior stays as in 3.10, including warnings), and maybe revisit this in 3.12.4 remaining items
@encukou Reproducer (somewhat):
Definition code:
class IsPyObject : public PyObject { public: IsPyObject(); virtual ~IsPyObject() { delete [] internal_data; --created_count; } virtual void somefunc() { internal_data[0]=1; } static void dealloc(PyObject* o) { delete static_cast<IsPyObject*>(o); } static int created_count; private: int* internal_data; // just something that can get corrupted }; int IsPyObject::created_count = 0; PyTypeObject ipotype = { PyObject_HEAD_INIT(NULL) "IsPyObject", // tp_name sizeof(IsPyObject), // tp_basicsize 0, // tp_itemsize IsPyObject::dealloc, // tp_dealloc }; IsPyObject::IsPyObject() { ob_type = &ipotype; Py_INCREF(this); internal_data = new int[50]; ++created_count; }and code to run it (goes in
_testcppext.cpptest_api_casts)if (PyType_Ready(&ipotype) < 0) return -1; IsPyObject* is_py_obj = new IsPyObject(); is_py_obj->somefunc(); Py_DECREF(is_py_obj); assert(IsPyObject::created_count==0);On my PC this doesn't crash, but
created_countis wrong on Python3.11 and right on Python 3.10. Various combinations of printing refcounts do cause it to crash, but it's honestly a bit arbitrary.However, the test_cppext test is only a compile test so doesn't run any of the module. Therefore it doesn't actually fail the test - it only fails when you build it yourself and run it manually.
Hopefully that's helpful though
Reacted by Petr ViktorinHowever, the test_cppext test is only a compile test so doesn't run any of the module. Therefore it doesn't actually fail the test - it only fails when you build it yourself and run it manually.
I'd appreciate a review for fixing that, if you have time: #94754
I can't test that entire PR because pywin32 doesn't build on 3.12 for unrelated reasons (still working on removing use of the the deprecated unicode apis!) but I cherry-picked 3faf13d into 3.11 and it does indeed work, thanks!
Reacted by Petr Viktorin- added a commit that references this issue
on Jul 14, 2022 The fix is now merged. Thank you for the help!
pywin32 has, since the 90s, used c++ to "inherit" from a PyObject in some cases. In short, it ends up something like:
With a layout like this, a
reinterpret_castbetween aMyObject *and aPyObject *will create invalid pointers.Consider something like the following:
This ends up being a macro which uses
_PyObject_CASTon the param passed toPy_DECREF. In 3.10, this uses a traditional "c" cast , but in 3.11 it turned into areinterpret_cast. In the above example, this causesPy_DECREFto be called with an invalidPyObject *- which typically doesn't crash at the time, but instead corrupts memory causing a difficult to diagnose crash some time later.Changing the code to use explicit
static_cast<>, or the old-school(PyObject *)cast makes things work, but it would be error prone as it would be easy to miss places where it's used via non-obvious nested macros. It also shouldn't be necessary to make this kind of change - c++ code which has worked for this long should continue to work.