Skip to content

gh-149816: Fix race in subtype_getweakref in free-threading build#150247

Open
Abhi210 wants to merge 2 commits into
python:mainfrom
Abhi210:gh-149816-61
Open

gh-149816: Fix race in subtype_getweakref in free-threading build#150247
Abhi210 wants to merge 2 commits into
python:mainfrom
Abhi210:gh-149816-61

Conversation

@Abhi210

@Abhi210 Abhi210 commented May 22, 2026

Copy link
Copy Markdown
Contributor

This PR resolves finding 61 from the weakref race tracking issue: "Racy weakref head load before incref in Objects/typeobject.c".

Description

In free-threaded builds, subtype_getweakref() had a race condition where the weakref list head (*weaklistptr) was loaded and immediately passed to Py_NewRef(). If another thread concurrently triggered weakref_dealloc(), the weakref could be freed before the incref, leading to a use-after-free (UAF).

This PR fixes the issue by:

  1. Wrapping the pointer load and incref sequence inside the existing LOCK_WEAKREFS(obj) / UNLOCK_WEAKREFS(obj) critical section.
  2. Replacing Py_NewRef() with _Py_TryIncref() to safely handle weakrefs whose refcount has reached zero during the race window.
  3. Returning Py_NewRef(Py_None) when the list is empty or the weakref is no longer live, preserving the existing API behavior.

Testing

Added regression tests in Lib/test/test_free_threading/test_weakref.py that:

  • Exercise obj.__weakref__ concurrently from reader and mutator threads synchronized with threading.Barrier.
  • Verify correct return values (None versus weakref.ref).
  • Check refcount stability across repeated accesses to detect leaks or incorrect incref/decref behavior.

AI Tool Disclosure:

Claude Code was used to help understand the relevant code paths and to improve the clarity and documentation of the proposed tests. It was not used to modify Objects/typeobject.c; all code changes and final decisions were made by me.

@python-cla-bot

python-cla-bot Bot commented May 22, 2026

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@nascheme

nascheme commented Jul 3, 2026

Copy link
Copy Markdown
Member

This looks like a real issue. Comments on PR:

  • use test.support.threading_helper.run_concurrently() rather than custom run_in_threads()
  • avoid using os.cpu_count(), might be large number on big machines, use a hard-coded small number of threads instead, should be sufficient to show issue
  • unit test file contains too much verbose comments, IMHO. This is typical of AI generated code in my experience. Don't explain what the code is doing if the code is clear (which it should be if all possible).
  • The if (*weaklistptr == NULL) branch should have braces to conform to modern CPython C style
  • The comment about LOCK_WEAKREFS() is overly verbose. I'd suggest: "Synchronize with weakref creation/destruction so the list head cannot be removed and freed between loading it and taking a reference."

@Abhi210

Abhi210 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Hello @nascheme! Thank you for the comments. I have resolved those issues. Let me know if there are any other changes to be made.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

22 free-threading race conditions

2 participants