Skip to content

[3.7] bpo-36389: _PyObject_IsFreed() now also detects uninitialized memory (GH-12770) #12788

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

Jump to bottom
Merged
merged 2 commits into from
Apr 11, 2019
Merged

[3.7] bpo-36389: _PyObject_IsFreed() now also detects uninitialized memory (GH-12770) #12788

merged 2 commits into from
Apr 11, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 11, 2019 •
edited by bedevere-bot
Loading

vstinner added 2 commits April 11, 2019 17:42
@vstinner
bpo-36389: _PyObject_IsFreed() now also detects uninitialized memory (G…
a1e1cea 
…H-12770)

Replace _PyMem_IsFreed() function with _PyMem_IsPtrFreed() inline
function. The function is now way more efficient, it became a simple
comparison on integers, rather than a short loop. It detects also
uninitialized bytes and "forbidden bytes" filled by debug hooks
on memory allocators.

Add unit tests on _PyObject_IsFreed().

(cherry picked from commit 2b00db6)
@vstinner
bpo-36389: Change PyMem_SetupDebugHooks() constants (GH-12782)
b06cbbe 
Modify CLEANBYTE, DEADDYTE and FORBIDDENBYTE constants: use 0xCD,
0xDD and 0xFD, rather than 0xCB, 0xBB and 0xFB, to use the same byte
patterns than Windows CRT debug malloc() and free().

(cherry picked from commit 4c409be)
@vstinner vstinner merged commit 9e23f0a into python:3.7 Apr 11, 2019
@vstinner vstinner deleted the object_freed37 branch April 29, 2019 13:56
asottile
asottile reviewed Jul 9, 2019
View reviewed changes
Include/pymem.h
@@ -55,8 +55,6 @@ PyAPI_FUNC(int) PyTraceMalloc_Untrack(
PyAPI_FUNC(PyObject*) _PyTraceMalloc_GetTraceback(
unsigned int domain,
uintptr_t ptr);

PyAPI_FUNC(int) _PyMem_IsFreed(void *ptr, size_t size);
Copy link
Contributor

Choose a reason for hiding this comment

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

(it's too late now) but should this have been backported? removing this symbol is ~technically an abi break (despite it being a "private" function)

packaging of 3.7.4 is complaining about disappearing symbols :(

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't provide any warranty on private symbols (prefixed by _Py or _PY): they can disappear anytime. The main issue was that we exposed it in the first place :-)

How do you check for symbols? My notes on the Python "stable" ABI: https://pythoncapi.readthedocs.io/stable_abi.html

@asottile
Copy link
Contributor

asottile commented Jul 9, 2019

We don't provide any warranty on private symbols

Doesn't surprise :)

How do you check for symbols?

debian lists the exported symbols of the .so file using dh_makeshlibs (the reasoning there being as packages upgrade and others are built against them, they can accurately represent the >= relationship with the absolute minimum version that's necessary -- instead of unnecessarily pinning to the latest version)

(it's actually quite annoying and probably the most time-consuming part of packaging python for me)

Here's (for example) an "update symbols" commit: deadsnakes/python3.7@c5aec48

(or one with a more involved set of changes): deadsnakes/python3.7@55bead2

@vstinner
Copy link
Member Author

vstinner commented Jul 9, 2019

Maybe I should modify internal headers to not export symbols like _Py_ResetForceASCII() which should not be used outside Python. It is only declared in internal headers: Include/internal/pycore_fileutils.h. It is declared with PyAPI_FUNC(). Maybe I should replace PyAPI_FUNC() with extern.

Do you have to list private symbols starting with _Py?

@asottile
Copy link
Contributor

asottile commented Jul 9, 2019

Do you have to list private symbols starting with _Py?

I'm not sure debian has a concept of "private by convention" (it might! I just don't know) -- to be honest I don't actually know too much about the makeshlibs utility but I'll do some research 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asottile asottile asottile left review comments

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vstinner @asottile @the-knights-who-say-ni @bedevere-bot