GeezerVison is showing github.com — _PyObject_CAST breaks code which assumed a static/dynamic casting · Issue #94731 · python/cpython
_PyObject_CAST breaks code which assumed a static/dynamic casting #94731Descriptionpywin32 has, since the 90s, used c++ to "inherit" from a PyObject in some cases. In short, it ends up something like:
class MyObject : public PyObject {
virtual void some_helper();
}
With a layout like this, a reinterpret_cast between a MyObject * and a PyObject * will create invalid pointers.
Consider something like the following:
void MyObject::some_helper() {
Py_DECREF(this);
}
This ends up being a macro which uses _PyObject_CAST on the param passed to Py_DECREF. In 3.10, this uses a traditional "c" cast , but in 3.11 it turned into a reinterpret_cast. In the above example, this causes Py_DECREF to be called with an invalid PyObject * - 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. Activityencukou commented on Jul 11, 2022 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? mhammond commented on Jul 11, 2022 ContributorAuthor More actions
@mhammond, do you happen to have a complete reproducer?
Yes, pywin32 on 3.11 - pythonwin will not start, see the linked issue. I'm confident a smaller repro could be constructed from the snippets above. encukou commented on Jul 11, 2022 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. da-woods commented on Jul 11, 2022 You don't actually need a cast here at all, because MyObject is a PyObject in C++ so converts directly without a cast.
You could do it easily in C++11 with std::enable_if and std::is_convertible but 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 helpers mhammond commented on Jul 11, 2022 ContributorAuthor More actions
You don't actually need a cast here at all, because MyObject is a PyObject in C++ so converts directly without a cast.
Right, which is why the example above is not doing a cast. The _Py_CAST in the Python headers are though. encukou commented on Jul 11, 2022 A C++11-only reproducer would help as well. da-woods commented on Jul 11, 2022 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_needed is only available if expr_type* can be converted to type directly. 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? da-woods commented on Jul 11, 2022 I can submit a PR for the overly elaborate but working mechanism if useful. Should be easy enough to create a test. mhammond commented on Jul 11, 2022 ContributorAuthor More actions 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? encukou commented on Jul 11, 2022 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. da-woods commented on Jul 11, 2022 @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.cpp test_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_count is 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 encukou commented on Jul 11, 2022
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.
I'd appreciate a review for fixing that, if you have time: #94754 encukou commented on Jul 13, 2022 mhammond commented on Jul 14, 2022 ContributorAuthor More actions 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! encukou commented on Jul 15, 2022 The fix is now merged. Thank you for the help!
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|