Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

bpo-35059: Convert _Py_Dealloc() to static inline function #10223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vstinner merged 2 commits into from
Oct 30, 2018
+24 −21
Merged

bpo-35059: Convert _Py_Dealloc() to static inline function #10223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d1ec2f
bpo-35059: Convert _Py_Dealloc() to static inline function
vstinner Oct 29, 2018
a20b426
Fix comment
vstinner Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions 21 Include/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,6 @@ PyAPI_FUNC(int) _PyTraceMalloc_NewReference(PyObject *op);
/* Py_TRACE_REFS is such major surgery that we call external routines. */
PyAPI_FUNC(void) _Py_NewReference(PyObject *);
PyAPI_FUNC(void) _Py_ForgetReference(PyObject *);
PyAPI_FUNC(void) _Py_Dealloc(PyObject *);
PyAPI_FUNC(void) _Py_PrintReferences(FILE *);
PyAPI_FUNC(void) _Py_PrintReferenceAddresses(FILE *);
PyAPI_FUNC(void) _Py_AddToAllObjects(PyObject *, int force);
Expand All @@ -790,15 +789,25 @@ static inline void _Py_ForgetReference(PyObject *op)
{
_Py_INC_TPFREES(op);
}
#endif /* !Py_TRACE_REFS */


#ifdef Py_LIMITED_API
PyAPI_FUNC(void) _Py_Dealloc(PyObject *);
Comment thread
serhiy-storchaka marked this conversation as resolved.

#ifndef Py_LIMITED_API
static inline void _Py_Dealloc_inline(PyObject *op)
Comment thread
serhiy-storchaka marked this conversation as resolved.
{
destructor dealloc = Py_TYPE(op)->tp_dealloc;
#ifdef Py_TRACE_REFS
_Py_ForgetReference(op);
#else
#define _Py_Dealloc(op) ( \
_Py_INC_TPFREES(op) _Py_COUNT_ALLOCS_COMMA \
(*Py_TYPE(op)->tp_dealloc)((PyObject *)(op)))

@vstinner vstinner Oct 30, 2018

Copy link
Copy Markdown
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 ;-)

_Py_INC_TPFREES(op);
#endif
#endif /* !Py_TRACE_REFS */
(*dealloc)(op);
}

# define _Py_Dealloc(op) _Py_Dealloc_inline(op)
#endif /* !defined(Py_LIMITED_API) */


static inline void _Py_INCREF(PyObject *op)
Expand Down
24 changes: 9 additions & 15 deletions 24 Objects/object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1964,14 +1964,6 @@ _Py_ForgetReference(PyObject *op)
_Py_INC_TPFREES(op);
}

void
_Py_Dealloc(PyObject *op)
{
destructor dealloc = Py_TYPE(op)->tp_dealloc;
_Py_ForgetReference(op);
(*dealloc)(op);
}

/* Print all live objects. Because PyObject_Print is called, the
* interpreter must be in a healthy state.
*/
Expand Down Expand Up @@ -2265,18 +2257,20 @@ _PyObject_AssertFailed(PyObject *obj, const char *msg, const char *expr,
Py_FatalError("_PyObject_AssertFailed");
}

#ifndef Py_TRACE_REFS
/* For Py_LIMITED_API, we need an out-of-line version of _Py_Dealloc.
Define this here, so we can undefine the macro. */

#undef _Py_Dealloc
void _Py_Dealloc(PyObject *);

void
_Py_Dealloc(PyObject *op)
{
_Py_INC_TPFREES(op) _Py_COUNT_ALLOCS_COMMA
(*Py_TYPE(op)->tp_dealloc)(op);
}
destructor dealloc = Py_TYPE(op)->tp_dealloc;
#ifdef Py_TRACE_REFS
_Py_ForgetReference(op);
#else
_Py_INC_TPFREES(op);
#endif
(*dealloc)(op);

@serhiy-storchaka serhiy-storchaka Oct 29, 2018

Copy link
Copy Markdown
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?

@vstinner vstinner Oct 30, 2018

Copy link
Copy Markdown
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).

@serhiy-storchaka serhiy-storchaka Oct 30, 2018

Copy link
Copy Markdown
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?

@vstinner vstinner Oct 30, 2018

Copy link
Copy Markdown
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!)

}

#ifdef __cplusplus
}
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.