Skip to content

gh-154431: Fix data race in sys.audithook#154462

Merged
ambv merged 1 commit into
python:mainfrom
sobolevn:issue-154431
Jul 22, 2026
Merged

gh-154431: Fix data race in sys.audithook#154462
ambv merged 1 commit into
python:mainfrom
sobolevn:issue-154431

Conversation

@sobolevn

@sobolevn sobolevn commented Jul 22, 2026

Copy link
Copy Markdown
Member

Test values are decreased, so this test can finish in a reasonable amount of time. It does not trigger in 100% of cases with TSAN enabled, but quite enought to notice its regressed failures in the future.

@sobolevn
sobolevn requested a review from kumaraditya303 July 22, 2026 09:54
@sobolevn sobolevn added needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Jul 22, 2026
@ambv
ambv merged commit 596cd5c into python:main Jul 22, 2026
59 checks passed
@miss-islington-app

Copy link
Copy Markdown

Thanks @sobolevn for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15.
🐍🍒⛏🤖

@bedevere-app

bedevere-app Bot commented Jul 22, 2026

Copy link
Copy Markdown

GH-154477 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label Jul 22, 2026
@bedevere-app

bedevere-app Bot commented Jul 22, 2026

Copy link
Copy Markdown

GH-154478 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jul 22, 2026
@sobolevn

Copy link
Copy Markdown
Member Author

Oh, @ambv looks like we can't really backport this. Because of the ABI change. What should we do? 🤔

sobolevn added a commit that referenced this pull request Jul 22, 2026
gh-154431: Fix data race in `sys.audithook` (GH-154462)
(cherry picked from commit 596cd5c)

Co-authored-by: sobolevn <mail@sobolevn.me>
@sobolevn

Copy link
Copy Markdown
Member Author

Maybe we can reuse interp->runtime->audit_hooks.mutex?

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apologies for not reviewing in time. I'm not so sure that a lock was the best choice here.

I think it would have been better to just initialize the list eagerly rather than lazily, which is simpler and solves the ABI problem. Alternatively, you could use a compare-exchange loop or a once flag.

@vstinner

Copy link
Copy Markdown
Member

The commit 596cd5c introduced a reference leak:

$ ./python -m test test_free_threading -R 3:3 -m test.test_free_threading.test_sys.SysModuleTest.test_sys_audit_hooks
...
test_free_threading leaked [104, 104, 104] references, sum=312
...

The test calls sys.addaudithook(_hook) 100 times, and so changes an internal global list of hooks. But by design (for security), there is no API to unregister hooks. Usually, what's be done for audit tests is to run them in a subprocess. For example, test_audit runs each test in a subprocess.

@sobolevn

Copy link
Copy Markdown
Member Author

Thank you @vstinner
I will fix that in #154494

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants