Skip to content

gh-153297: Fix data race on assigning __name__ and __qualname__ to functions on FT builds - #153335

Closed
sobolevn wants to merge 6 commits into
python:mainfrom
sobolevn:issue-153297
Closed

gh-153297: Fix data race on assigning __name__ and __qualname__ to functions on FT builds#153335
sobolevn wants to merge 6 commits into
python:mainfrom
sobolevn:issue-153297

Conversation

@sobolevn

@sobolevnsobolevn commented Jul 8, 2026

Copy link
Copy Markdown
Member

@sobolevn
sobolevn requested a review from colesburyJuly 8, 2026 13:03
@sobolevn

Copy link
Copy Markdown
MemberAuthor

13

1 test failed:
test_hashlib

failure is not related. Refs #153201

@kumaraditya303

Copy link
Copy Markdown
Contributor

I think there are places where these are read without critical section so it doesn't fix all the cases.

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Yes, like here :(

// Objects/genobject.cassert(func->func_name!=NULL);
gen->gi_name=Py_NewRef(func->func_name);

It now uses a critical section on func.
And the second fix is for function.__code__, which also now uses critical sections, when accessing __name__.

But, initialize_locals in ceval.c seem safe? I am not sure if I should touch it in any way.

@devdanzin

devdanzin commented Jul 11, 2026

Copy link
Copy Markdown
Member

This PR adds critical sections to __name__, __qualname__, and __code__ — but func_set_defaults / func_get_defaults (and func_set_kwdefaults / func_get_kwdefaults) have the identical Py_XSETREF-setter / Py_NewRef-getter pattern and don't appear to be covered. On a free-threaded build, racing f.__defaults__ read/write segfaults the same way (via a negative refcount in the debug build; func_set_defaults in release):

importsys, threading, timeassertnotsys._is_gil_enabled()
classM: passdeftarget(): passtarget.__defaults__= (M(),)
stop=Falsedefchurn():
whilenotstop:
target.__defaults__= (M(),)
defread():
whilenotstop:
d=target.__defaults__; deldts= [threading.Thread(target=churn) for_inrange(4)]
ts+= [threading.Thread(target=read) for_inrange(8)]
fortints: t.start()
time.sleep(5); stop=Truefortints: t.join()

Debug FT: Python/brc.c:62 _Py_NegativeRefcount: object has negative ref count. Release FT: SIGSEGV in func_set_defaults (funcobject.c) ← PyObject_SetAttr. Worth extending this PR (or a follow-up) to __defaults__/__kwdefaults__ so the whole getsetlist is covered in one pass.

ETA: __annotations__ is also structurally unprotected and not touched by this PR (its getter goes through the lock-free lazy func_get_annotation_dict + func_annotate), but I did not manage to reproduce a crash via __annotations__ in 5 runs the way __defaults__/__kwdefaults__ do. The lazy-annotate path evidently has a narrower window. Flagging it as structurally-suspect but unconfirmed, in case it's worth covering defensively while touching this area.

Investigation and draft by Claude Code (Opus 4.8).

@kumaraditya303

Copy link
Copy Markdown
Contributor

I created #154826 to fix this properly, it uses stop the world pause and changes the attribute and clears the version atomically which is important and missing from this PR.

Sign up for freeto 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.

3 participants

@sobolevn@kumaraditya303@devdanzin