Skip to content

Fix free-threading bugs in the C extension - #467

Open
oschwald wants to merge 13 commits into
greg/stf-1956from
greg/stf-1957
Open

Fix free-threading bugs in the C extension#467
oschwald wants to merge 13 commits into
greg/stf-1956from
greg/stf-1957

Conversation

@oschwald

@oschwald oschwald commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes two free-threading bugs in the C extension. Stacked on #466; review only the commits in this PR.

  • Deadlock. ReaderIter_next called ipaddress.ip_network while it held the read lock. A close() from a signal handler on the same thread, or from another thread during a garbage collection that the callback started, waited forever. The lock is now released before any Python code runs.

  • Heap corruption. Two threads that advanced the same iterator could take and free the same pending record. Each next() call now holds a critical section on the iterator.

  • CI. No job ran a free-threaded interpreter, so the locks compiled to no-ops in every test run. A new 3.14t tox environment runs on each CI platform, including macOS.

A subprocess test with a timeout closes the reader from ip_network, and a free-threaded test shares one iterator between eight threads and checks that each network comes out once. Both fail without the fixes.

STF-1957

🤖 Generated with Claude Code

Copilot AI balanced review requested due to automatic review settings October 3, 2026 15:52
@oschwald oschwald mentioned this pull request Oct 3, 2026
Open
Copilot AI reviewed Oct 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: a782e643-4c0e-4b15-824f-938abeed5d95

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI balanced review requested due to automatic review settings October 3, 2026 16:17
Copilot AI reviewed Oct 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI balanced review requested due to automatic review settings October 3, 2026 16:38
Copilot AI reviewed Oct 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@oschwald oschwald mentioned this pull request Oct 3, 2026
Closed
Copilot AI balanced review requested due to automatic review settings October 5, 2026 15:32
Copilot AI reviewed Oct 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

oschwald and others added 9 commits October 5, 2026 16:14
@oschwald @claude
Fix memory leaks and a use-after-free in the C extension
ea929d0 
The Reader, Metadata and iterator types are heap types, so each instance
holds a reference to its type. The dealloc functions did not release
that reference, so each object leaked one reference to its type, and
the types were never freed.

A second Reader_init did not close the open database, so it leaked. It
also left existing iterators with records that point into the old
database, so the next step of such an iterator read freed memory.

A second init now reopens the reader, as in the pure Python reader. It
closes the old database under the write lock before it opens the new
one, so a failed open leaves the reader closed. Each open increments a
generation count, and an iterator from an older generation raises
ValueError.

Metadata_init also accepted a second init, which leaked the old field
values. Refuse it with a ValueError.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Pass a Py_ssize_t length to Py_BuildValue
521d18e 
The module defines PY_SSIZE_T_CLEAN, so the y# format reads the length
as a Py_ssize_t. ReaderIter_next passed an int, which is undefined
behavior on 64-bit platforms: Py_BuildValue reads 8 bytes from a
4-byte argument.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Fix a reference leak when a map key fails to decode
61c34af 
from_map built the dict, then returned NULL without releasing it when a
key failed to decode, for example a map key that is not valid UTF-8. The
sibling path for a failed value already released the dict. The cyclic
garbage collector does not free an object with a leaked reference, so
each failed lookup kept one empty dict, about 64 bytes.

The new test makes 2,000 failed lookups and checks the memory that
tracemalloc reports. Before the fix, the lookups kept about 128 KB.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Check the PyDict_SetItem result in from_map
d978294 
from_map ignored a failure from PyDict_SetItem, such as a MemoryError
while the dict resizes. It then returned the dict with the exception
still set, and the caller raised SystemError. Now it releases the dict
and returns NULL, so the original exception reaches the caller.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Reject a map key that is not a string
09f6bd7 
from_map read every map key from the utf8_string member of the entry
union. libmaxminddb does not check the key type, so a database with a
key of another type, such as a uint16, made from_map read an integer as
a pointer. The process crashed with a segmentation fault.

Raise InvalidDatabaseError for a key that is not a UTF-8 string.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Decode uint32 values with PyLong_FromUnsignedLong
f76a39e 
from_entry_data_list passed uint32 values to PyLong_FromLong. On
platforms where a C long has 32 bits, such as Windows and 32-bit Linux,
a value of 2**31 or more became negative, for example an ASN of
4200000000. The pure Python reader returns the correct value.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Fix a reference leak on a corrupt metadata type
155bfc3 
Reader.metadata() returned NULL without releasing the decoded object when
it was not a dict. libmaxminddb validates the metadata when it opens the
database, so an opened database reaches this path only through a bug, but
the refcount handling was still wrong.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@oschwald @claude
Check the result of Reader_close
150a52e 
Reader__exit__ and Reader_dealloc called Reader_close and ignored the
result. Each successful close leaked a reference to None, which matters
before Python 3.12, where None is not immortal. On free-threaded builds,
a failure to take the write lock left an exception set: __exit__ hid it
behind a successful return, and dealloc left it pending for unrelated
code to find.

__exit__ now returns the result of Reader_close. dealloc no longer calls
Reader_close. It closes the database directly, without the lock. At a
reference count of 0 no other thread can use the reader, because each
iterator holds a reference to it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Build Metadata in tp_new
636b656 
Metadata is an immutable value object, but Metadata_init set its fields.
That allowed a Metadata with NULL fields, after Metadata.__new__ or a
failed init, and a second init. Each state needed its own guard: a
reinit check, a critical section on free-threaded builds, and
Py_XDECREF in dealloc.

Metadata_new now parses the arguments and sets every field, and the type
has no tp_init. Each Metadata then has all fields set, and a second
__init__ call changes nothing, because object.__init__ ignores the
arguments when a type overrides tp_new. Metadata.__new__ with no
arguments now raises TypeError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 16:29
Copilot AI reviewed Oct 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@oschwald @claude
Reopen the pure Python reader like the C extension
0b7589f 
A second __init__ on the pure Python Reader replaced the buffer, but it
did not close the old one. An iterator from before the second init then
walked the new database with node numbers from the old one, and
returned networks that are not in either database.

The C extension now closes the old database and stops such an
iterator. Do the same here: __init__ closes the old buffer once the new
one is loaded and increments a generation count. An iterator from an
older generation raises the same ValueError. A count is needed, because
a source can return the same buffer object again: BytesIO.read() does
after seek(0). The check is one integer comparison for each node during
iteration. Lookups do not change.

The reinitialization tests now run for every mode of both readers, so
the two keep the same behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
oschwald and others added 3 commits October 5, 2026 16:52
@oschwald @claude
Release the read lock before building the network
8527eec 
ReaderIter_next held the read lock while it called ipaddress.ip_network,
which runs Python code. That caused two hangs on free-threaded builds:

- If the code closed the reader on the same thread, for example from a
  signal handler, close() waited for the write lock that the thread's
  own read lock blocked.
- If the code ran the garbage collector while another thread waited in
  close() for the write lock, the collector stopped the world and waited
  for that thread, which waited for the read lock.

The network uses only the iterator's own record, so release the lock
after the record is decoded. Now no thread runs Python code while it
holds the lock. A SIGALRM handler that closes the reader during
iteration, and a wrapped ip_network that calls gc.collect() while
another thread calls close(), both hung before this change and work
after it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Let one thread at a time advance an iterator
3880049 
ReaderIter_next takes records off the iterator's pending list and adds
their children while it holds only the shared read lock. On
free-threaded builds, two threads that called next() on the same
iterator could take the same record and both free it. The process
aborted with heap corruption.

Hold a critical section on the iterator for each next() call. With the
GIL, the list changes already run without interruption.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@oschwald @claude
Test on free-threaded Python in CI
f3c264c 
No CI job ran a free-threaded interpreter, so the reader locks compiled
to no-ops in every test run. The tests for the lock lifetime, the lock
release in the iterator, and the shared iterator could not fail, and
macOS, where a zeroed pthread_rwlock_t is invalid, was never tested.

Add a 3.14t tox environment and run it on each CI platform.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 17:00
Copilot AI reviewed Oct 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

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

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@oschwald