Skip to content

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

Merged
vstinner merged 2 commits into
python:3.7from
vstinner:object_freed37
Apr 11, 2019
Merged

[3.7] bpo-36389: _PyObject_IsFreed() now also detects uninitialized memory (GH-12770)#12788
vstinner merged 2 commits into
python:3.7from
vstinner:object_freed37

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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
Comment thread Include/pymem.h
unsigned int domain,
uintptr_t ptr);

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

@asottile asottile Jul 9, 2019

Copy link
Copy Markdown
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 :(

@vstinner vstinner Jul 9, 2019

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.

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

asottile commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

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

vstinner commented Jul 9, 2019

Copy link
Copy Markdown
Member Author

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

asottile commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

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

1 more reviewer

@asottile asottile asottile left review comments

Reviewers whose approvals may not affect merge requirements

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