jcrist · GitHub

added 3 commits

July 14, 2022 13:37
This fixes issues caused by Python 3.11 changing some internal APIs
(mostly around GC implementation).

@jcrist

@jcrist

jcrist added a commit that referenced this pull request

Jul 21, 2022
This fixes a segfault introduced in #146. This bug was never released.
- Before Python 3.11, calling `PyObject_GC_Del` worked the same as
`PyObject_Del`, where it was functionally equivalent to `free`, only
treating the argument as an opaque pointer.
- In Python 3.11, `PyObject_GC_Del` accesses the `ob_type` field to
determine the size of the GC header. This didn't play well with our
freelist implementation.
- To work around this, #146 moved to keeping the `ob_type` field on
objects in the freelist, only decref'ing their type when the memory is
used, or when it is cleared by `Struct_freelist_clear`.
- However, `Struct_freelist_clear` is called during a GC traversal. In
rare circumstances, an object stored in our freelist can contain the
only reference to a `StructMeta` type object. Decref'ing `ob_type` here
would lead to that `StructMeta` object being freed *during the GC
traversal*. Freeing GC types during a GC traversal can do weird things
with the GC linked list, leading to segfaults.
This bug is rare, since in most cases Struct types are defined at the
top level of a module, and live for the duration of the program. It also
requires the GC to be called at the proper time and have the freelist in
the proper order to trigger the bug. All of this is complicated.
The fix here is to:
- Always decref the type immediately in `Struct_dealloc`
- Replace the `ob_type` fields for struct memory blocks stored in our
freelist with references to known non-heap types. `PyObject_GC_Del` only
checks `tp_flags` on the type, so the actual type doesn't really matter.
We use a non-heap type so that we know the type reference will live for
the duration of the program, and no incref/decref behavior will be
needed.
This was a complicated bug to track down though, in the future we should
re-evaluate whether the freelist is worth it for actual programs. It
gives a measurable boost for microbenchmarks (which many users will use
to judge performance), but I'm not positive it really matters for real
world use.

Merged

jcrist added a commit that referenced this pull request

Jul 21, 2022
This fixes a segfault introduced in #146. This bug was never released.
- Before Python 3.11, calling `PyObject_GC_Del` worked the same as
`PyObject_Del`, where it was functionally equivalent to `free`, only
treating the argument as an opaque pointer.
- In Python 3.11, `PyObject_GC_Del` accesses the `ob_type` field to
determine the size of the GC header. This didn't play well with our
freelist implementation.
- To work around this, #146 moved to keeping the `ob_type` field on
objects in the freelist, only decref'ing their type when the memory is
used, or when it is cleared by `Struct_freelist_clear`.
- However, `Struct_freelist_clear` is called during a GC traversal. In
rare circumstances, an object stored in our freelist can contain the
only reference to a `StructMeta` type object. Decref'ing `ob_type` here
would lead to that `StructMeta` object being freed *during the GC
traversal*. Freeing GC types during a GC traversal can do weird things
with the GC linked list, leading to segfaults.
This bug is rare, since in most cases Struct types are defined at the
top level of a module, and live for the duration of the program. It also
requires the GC to be called at the proper time and have the freelist in
the proper order to trigger the bug. All of this is complicated.
The fix here is to:
- Always decref the type immediately in `Struct_dealloc`
- Replace the `ob_type` fields for struct memory blocks stored in our
freelist with references to known non-heap types. `PyObject_GC_Del` only
checks `tp_flags` on the type, so the actual type doesn't really matter.
We use a non-heap type so that we know the type reference will live for
the duration of the program, and no incref/decref behavior will be
needed.
This was a complicated bug to track down though, in the future we should
re-evaluate whether the freelist is worth it for actual programs. It
gives a measurable boost for microbenchmarks (which many users will use
to judge performance), but I'm not positive it really matters for real
world use.

jcrist added a commit that referenced this pull request

Jul 21, 2022
This fixes a segfault introduced in #146. This bug was never released.
- Before Python 3.11, calling `PyObject_GC_Del` worked the same as
`PyObject_Del`, where it was functionally equivalent to `free`, only
treating the argument as an opaque pointer.
- In Python 3.11, `PyObject_GC_Del` accesses the `ob_type` field to
determine the size of the GC header. This didn't play well with our
freelist implementation.
- To work around this, #146 moved to keeping the `ob_type` field on
objects in the freelist, only decref'ing their type when the memory is
used, or when it is cleared by `Struct_freelist_clear`.
- However, `Struct_freelist_clear` is called during a GC traversal. In
rare circumstances, an object stored in our freelist can contain the
only reference to a `StructMeta` type object. Decref'ing `ob_type` here
would lead to that `StructMeta` object being freed *during the GC
traversal*. Freeing GC types during a GC traversal can do weird things
with the GC linked list, leading to segfaults.
This bug is rare, since in most cases Struct types are defined at the
top level of a module, and live for the duration of the program. It also
requires the GC to be called at the proper time and have the freelist in
the proper order to trigger the bug. All of this is complicated.
The fix here is to:
- Always decref the type immediately in `Struct_dealloc`
- Replace the `ob_type` fields for struct memory blocks stored in our
freelist with references to known non-heap types. `PyObject_GC_Del` only
checks `tp_flags` on the type, so the actual type doesn't really matter.
We use a non-heap type so that we know the type reference will live for
the duration of the program, and no incref/decref behavior will be
needed.
This was a complicated bug to track down though, in the future we should
re-evaluate whether the freelist is worth it for actual programs. It
gives a measurable boost for microbenchmarks (which many users will use
to judge performance), but I'm not positive it really matters for real
world use.

Read the original on github.com ↗