Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 38 additions & 1 deletion Objects/obmalloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,45 @@ _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 access from other threads 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*)) {
// Use memcpy to avoid strict-aliasing issues. However, we probably
// still have unavoidable strict-aliasing issues with
// _Py_atomic_store_ptr_relaxed here.
void *word;
memcpy(&word, (char*)ptr + i, sizeof(void*));
_Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), word);
Copy link
Member

Choose a reason for hiding this comment

The 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
_Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), word);
_Py_atomic_store_ptr_relaxed((void**)((char*)newp + i), (void *)((char *)ptr + 1));

(If it can't be, it probably deserves a comment.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that the memcpy avoids accessing data the pointers of an incorrect size or strict aliasing issues. For example, we read ob_flags, ob_mutex, ob_gc_bits, and ob_ref_local together as a 64-bit load (on 64-bit systems).

I guess there's still the same issue when we write the data with _Py_atomic_store_ptr_relaxed, so maybe it doesn't matter.

From what I can tell, UBSan and ASan don't check for this kind of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
4 changes: 0 additions & 4 deletions Tools/tsan/suppressions_free_threading.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,3 @@ race_top:_PyObject_TryGetInstanceAttribute

# https://gist.github.com/mpage/6962e8870606cfc960e159b407a0cb40
thread:pthread_create

# PyObject_Realloc internally does memcpy which isn't atomic so can race
# with non-locking reads. See #132070
race:PyObject_Realloc
Loading