Skip to content

gh-129983: fix data race in compile_template in sre.c #130015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Feb 12, 2025
Merged

gh-129983: fix data race in compile_template in sre.c #130015

merged 9 commits into from
Feb 12, 2025
+10 −1

Conversation

tom-pytel
Copy link
Contributor

@tom-pytel tom-pytel commented Feb 11, 2025 •
edited
Loading

I am assuming the conversation was resolved on the side of the atomic?

I am not sure about the test added here for 3 reasons:

  1. Need to turn off all the other re tests because not everything there is free-thread safe yet.
  2. Need to make sure nothing uses re.sub() before this test so that re._compile_template doesn't get cached yet.
  3. The test itself is very non-deterministic by its nature, sometimes it will success when it should fail.

The other option would be to make a separate test file just for this case? Overkill I think. So opinion on this requested, maybe don't need test for this?

@bedevere-app bedevere-app bot mentioned this pull request Feb 11, 2025
Closed
colesbury
colesbury reviewed Feb 11, 2025
View reviewed changes
Modules/_sre/sre.c Outdated
@@ -1167,13 +1167,21 @@ compile_template(_sremodulestate *module_state,
PatternObject *pattern, PyObject *template)
{
/* delegate to Python code */
PyObject *func = module_state->compile_template;
PyObject *func = FT_ATOMIC_LOAD_PTR_RELAXED(module_state->compile_template);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use FT_ATOMIC_LOAD_PTR here.

FT_ATOMIC_LOAD_PTR_ACQUIRE would probably also be fine, but I think symmetry between the load and store is generally a good idea.

@colesbury
Copy link
Contributor

I think we can leave the test out for now. I briefly looked at this yesterday, and the other tests in test_re would catch this when run with --parallel-threads. The main problem, as you probably noticed, is that lots of the tests would fail because catch_warnings is not thread-safe (even with the GIL). I think that should be fixed relatively soon with #130010. When that's ready, we can enable test_re in TSAN_PARALLEL_TESTS. I'm not too worried about regressions in the meantime.

colesbury
colesbury approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks!

@colesbury
Copy link
Contributor

@kumaraditya303 - is this okay with you?

@tom-pytel
Copy link
Contributor Author

Thanks!

np :)

kumaraditya303
kumaraditya303 approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@kumaraditya303 kumaraditya303 added the needs backport to 3.13 bugs and security fixes label Feb 12, 2025
@kumaraditya303 kumaraditya303 merged commit 3cf68cd into python:main Feb 12, 2025
50 checks passed
@miss-islington-app
Copy link

Thanks @tom-pytel for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @tom-pytel and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3cf68cdd3e1809df4e426c61f6990de63747ec6f 3.13

kumaraditya303 pushed a commit to kumaraditya303/cpython that referenced this pull request Feb 12, 2025
@tom-pytel @kumaraditya303
pythongh-129983: fix data race in compile_template in sre.c (python#1…
Loading
Loading status checks…
6c54f89 
…30015)

(cherry picked from commit 3cf68cd)
kumaraditya303 added a commit that referenced this pull request Feb 12, 2025
@kumaraditya303 @tom-pytel
[3.13] gh-129983: fix data race in compile_template in sre.c (#130038)
4cb251d 
gh-129983: fix data race in compile_template in sre.c (#130015)

(cherry picked from commit 3cf68cd)

Co-authored-by: Tomasz Pytel <tompytel@gmail.com>
@tom-pytel
Copy link
Contributor Author

Anyone want backport or should I do it?

@kumaraditya303
Copy link
Contributor

Anyone want backport or should I do it?

I did that already #130038

@hugovk hugovk removed the needs backport to 3.13 bugs and security fixes label Feb 17, 2025
@tom-pytel tom-pytel deleted the fix-issue-129983 branch March 4, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesbury colesbury

@kumaraditya303 kumaraditya303

Assignees

@kumaraditya303 kumaraditya303

Labels
topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tom-pytel @colesbury @kumaraditya303 @hugovk