-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-132070: Fix PyObject_Realloc thread-safety in free threaded Python #143441
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -307,8 +307,42 @@ _PyObject_MiRealloc(void *ctx, void *ptr, size_t nbytes) | |||||
| { | ||||||
| #ifdef Py_GIL_DISABLED | ||||||
| _PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET(); | ||||||
| // Implement our own realloc logic so that we can copy PyObject header | ||||||
| // in a thread-safe way. | ||||||
| size_t size = mi_usable_size(ptr); | ||||||
| if (nbytes <= size && nbytes >= (size / 2) && nbytes > 0) { | ||||||
| return ptr; | ||||||
| } | ||||||
|
|
||||||
| mi_heap_t *heap = tstate->mimalloc.current_object_heap; | ||||||
| return mi_heap_realloc(heap, ptr, nbytes); | ||||||
| void* newp = mi_heap_malloc(heap, nbytes); | ||||||
| if (newp == NULL) { | ||||||
| return NULL; | ||||||
| } | ||||||
|
|
||||||
| // Free threaded Python allows safe access to the PyObject reference count | ||||||
| // fields for a period of time after the object is freed (see InternalDocs/qsbr.md). | ||||||
| // These fields are typically initialized by PyObject_Init() using relaxed | ||||||
| // atomic stores. We need to copy these fields in a thread-safe way here. | ||||||
| // We use the "debug_offset" to determine how many bytes to copy -- it | ||||||
| // includes the PyObject header and plus any extra pre-headers. | ||||||
| size_t offset = heap->debug_offset; | ||||||
| assert(offset % sizeof(void*) == 0); | ||||||
|
|
||||||
| size_t copy_size = (size < nbytes ? size : nbytes); | ||||||
| if (copy_size >= offset) { | ||||||
| for (size_t i = 0; i != offset; i += sizeof(void*)) { | ||||||
| void *word; | ||||||
| memcpy(&word, (char*)ptr + i, sizeof(void*)); | ||||||
| _Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), word); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for the intermediate word? And why the memcpy instead of just a cast + deref + assignment? Can't this just be:
Suggested change
(If it can't be, it probably deserves a comment.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking was that the I guess there's still the same issue when we write the data with From what I can tell, UBSan and ASan don't check for this kind of thing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a comment |
||||||
| } | ||||||
| _mi_memcpy((char*)newp + offset, (char*)ptr + offset, copy_size - offset); | ||||||
| } | ||||||
| else { | ||||||
| _mi_memcpy(newp, ptr, copy_size); | ||||||
| } | ||||||
| mi_free(ptr); | ||||||
| return newp; | ||||||
| #else | ||||||
| return mi_realloc(ptr, nbytes); | ||||||
| #endif | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.