Skip to content

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 29, 2018

Convert _Py_Dealloc() macro into a static inline function. Moreover,
it is now also defined as a static inline function if Py_TRACE_REFS
is defined.

https://bugs.python.org/issue35059

Convert _Py_Dealloc() macro into a static inline function. Moreover,
it is now also defined as a static inline function if Py_TRACE_REFS
is defined.
@vstinner
Copy link
Member Author

@methane: Py_INCREF and Py_DECREF have been converted to static inline functions. So I came back to _Py_Dealloc() as you asked me, and it seems like I found a solution to handle properly this one:

  • " PyAPI_FUNC(void) _Py_Dealloc(PyObject *); " is now always declared the same way with any C define and it is now always implemented in object.c at the same place (previously, there were two implementations, depending if Py_TRACE_REFS)
  • Name the inlined flavor "_Py_Dealloc_inline" and use #define _Py_Dealloc(op) _Py_Dealloc_inline(op), so the name doesn't conflict depending if Py_LIMITED_API is defined or not

@vstinner vstinner requested a review from benjaminp October 29, 2018 19:07
#endif
(*dealloc)(op);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that it is safe to use a borrowed reference taken before calling _Py_INC_TPFREES?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My PR doesn't change anything related to that. See the old code at line 1970: it does the same. I would say that it's safer to read dealloc before _Py_ForgetReference() or _Py_INC_TPFREES() than reading it after (as previously done in the old code at line 2277).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Py_INC_TPFREES decrements the refcount. Can it trigger executing arbitrary code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Py_ForgetReference() and _Py_INC_TPFREES() is called before calling dealloc() since forever. If you consider that it's a bug, you should write a separated PR (and maybe open an issue).

I have no idea if this case can occur. I can only say that my PR only moves the code, it doesn't change the code. Since nobody ever reported a crash, I don't think that dealloc can become a dangling pointer.

Please see the discussion about Py_TYPE() borrowed reference:
https://mail.python.org/mm3/archives/list/capi-sig@python.org/thread/V5EMBIIJFJGJGBQPLCFFXCHAUFNTA45H/

Especially my second email: "I succeeded to get a dangling pointer to a deallocated type object using Py_TYPE()."

But to get a dangling pointer to a type, you have to remove all references to all instances and to the type itself. It seems doable, but also very unlikely in practice.

Again, if you consider that it's a bug, I suggest to open a separated issue and fix the bug in all stable branches. (And I have no strong opinion to know if it's a bug or not :-))

My discussion on capi-sig is able PyTypeObject* pointer, but here we are talking about dealloc which is a function pointer. To get a dangling pointer, you would have to somehow remove the function (code) from the memory. Maybe it's possible using ctypes, but it seems very very unlikely :-) (Don't do that at home, or very bad things can happen to you!)

@vstinner vstinner merged commit 3c09dca into python:master Oct 30, 2018
@vstinner vstinner deleted the py_dealloc branch October 30, 2018 13:48
@vstinner
Copy link
Member Author

@serhiy-storchaka: You asked interesting questions, but I merged my change anyway.

For the coding style, if you want to change it: please write a separated PR to reformat all "static inline" at once, not only these ones.

For _Py_Dealloc() race condition: if you consider that it's a bug, please open a new issue. My commit doesn't change the code, it only moves the code.

#else
#define _Py_Dealloc(op) ( \
_Py_INC_TPFREES(op) _Py_COUNT_ALLOCS_COMMA \
(*Py_TYPE(op)->tp_dealloc)((PyObject *)(op)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhiy-storchaka: Oh, I forgot to mention that my PR changes this code to read dealloc before _Py_INC_TPFREES(), it might make the code safer ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants