Repository navigation
Data race in compile_template in sre.c #129983
Description
Activity
This fixes the issue. Let me know if you want me to send up. Or if you prefer it can be done with mutex (but uglier).
$ git diff main diff --git a/Modules/_sre/sre.c b/Modules/_sre/sre.c index 0d8d4843d33..64ef6b202e5 100644 --- a/Modules/_sre/sre.c +++ b/Modules/_sre/sre.c @@ -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); if (func == NULL) { func = PyImport_ImportModuleAttrString("re", "_compile_template"); if (func == NULL) { return NULL; } +#ifdef Py_GIL_DISABLED + PyObject *other_func = NULL; + if (!_Py_atomic_compare_exchange_ptr(&module_state->compile_template, &other_func, func)) { + Py_DECREF(func); + func = other_func; + } +#else Py_XSETREF(module_state->compile_template, func); +#endif } PyObject *args[] = {(PyObject *)pattern, template};
Reproducer:
import re import threading def test(b): b.wait() re.sub(r"(\d+)", r"\1kg", "Weight: 50") def check(funcs, *args): barrier = threading.Barrier(len(funcs)) thrds = [] for func in funcs: thrd = threading.Thread(target=func, args=(barrier, *args)) thrds.append(thrd) thrd.start() for thrd in thrds: thrd.join() if __name__ == "__main__": while True: check([test] * 40)
Also, while testing this I ran into this error exactly once and no more, though I don't think it has anything to do with this particular issue or fix:
ThreadSanitizer:DEADLYSIGNAL ==470981==ERROR: ThreadSanitizer: SEGV on unknown address 0x7f5cda2d0320 (pc 0x7f5d1447ed0f bp 0x00000000000d sp 0x7f5c221bf980 T471055) ==470981==The signal is caused by a WRITE memory access. #0 unsigned long long func_cas<unsigned long long>(unsigned long long volatile*, unsigned long long, unsigned long long) ../../../../src/libsanitizer/tsan/tsan_interface_atomic.cpp:97 (libtsan.so.0+0x7ed0f) #1 AtomicCAS<long long unsigned int> ../../../../src/libsanitizer/tsan/tsan_interface_atomic.cpp:422 (libtsan.so.0+0x7ed0f) #2 __tsan_atomic64_compare_exchange_strong ../../../../src/libsanitizer/tsan/tsan_interface_atomic.cpp:786 (libtsan.so.0+0x86ac2) #3 _Py_atomic_compare_exchange_ssize Include/cpython/pyatomic_gcc.h:130 (python+0x2235f2) #4 _Py_TryIncRefShared Include/internal/pycore_object.h:582 (python+0x2235f2) #5 _Py_TryIncrefCompare Include/internal/pycore_object.h:602 (python+0x2235f2) #6 _Py_TryXGetRef Include/internal/pycore_object.h:652 (python+0x22d0df) #7 _Py_dict_lookup_threadsafe Objects/dictobject.c:1543 (python+0x22d0df) #8 dict_subscript Objects/dictobject.c:3392 (python+0x22d65b) #9 PyObject_GetItem Objects/abstract.c:158 (python+0x14ca14) #10 _PyEval_EvalFrameDefault Python/generated_cases.c.h:62 (python+0x41b7c2) #11 _PyEval_EvalFrame Include/internal/pycore_ceval.h:116 (python+0x46f61d) #12 _PyEval_Vector Python/ceval.c:1725 (python+0x46f61d) #13 _PyFunction_Vectorcall Objects/call.c:413 (python+0x1838dc) #14 _PyObject_VectorcallTstate Include/internal/pycore_call.h:167 (python+0x18b5e3) #15 method_vectorcall Objects/classobject.c:72 (python+0x18b5e3) #16 _PyVectorcall_Call Objects/call.c:273 (python+0x187428) #17 _PyObject_Call Objects/call.c:348 (python+0x187956) #18 PyObject_Call Objects/call.c:373 (python+0x1879db) #19 thread_run Modules/_threadmodule.c:354 (python+0x6669ab) #20 pythread_wrapper Python/thread_pthread.h:242 (python+0x57bf0d) #21 __tsan_thread_start_func ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:959 (libtsan.so.0+0x2e5ff) #22 start_thread nptl/pthread_create.c:442 (libc.so.6+0x94ac2) #23 <null> <null> (libc.so.6+0x12684f) ThreadSanitizer can not provide additional info. SUMMARY: ThreadSanitizer: SEGV ../../../../src/libsanitizer/tsan/tsan_interface_atomic.cpp:97 in unsigned long long func_cas<unsigned long long>(unsigned long long volatile*, unsigned long long, unsigned long long) ==470981==ABORTINGThanks - pease send a PR!
I think the much simpler solution would be to initialize
compile_templatein module exec in this casesre_exec, then there will not be any thread safety issues as module exec is exclusively executed first before any code.Reacted by SokratoI think the much simpler solution would be to initialize
compile_templatein module exec in this casesre_execThe code in
compile_template()is a lazy import becausereimports_sre, which is why you can't initializecompile_templateinsre_exec. You can however importreinsre_execand access it incompile_template(), do you want this one?$ git diff main diff --git a/Modules/_sre/sre.c b/Modules/_sre/sre.c index 0d8d4843d33..c2b1be957f5 100644 --- a/Modules/_sre/sre.c +++ b/Modules/_sre/sre.c @@ -376,7 +376,7 @@ typedef struct { PyTypeObject *Match_Type; PyTypeObject *Scanner_Type; PyTypeObject *Template_Type; - PyObject *compile_template; // reference to re._compile_template + PyObject *re_module; } _sremodulestate; static _sremodulestate * @@ -1167,17 +1167,13 @@ compile_template(_sremodulestate *module_state, PatternObject *pattern, PyObject *template) { /* delegate to Python code */ - PyObject *func = module_state->compile_template; + PyObject *func = PyObject_GetAttrString(module_state->re_module, "_compile_template"); if (func == NULL) { - func = PyImport_ImportModuleAttrString("re", "_compile_template"); - if (func == NULL) { - return NULL; - } - Py_XSETREF(module_state->compile_template, func); + return NULL; } - PyObject *args[] = {(PyObject *)pattern, template}; PyObject *result = PyObject_Vectorcall(func, args, 2, NULL); + Py_DECREF(func); if (result == NULL && PyErr_ExceptionMatches(PyExc_TypeError)) { /* If the replacement string is unhashable (e.g. bytearray), @@ -3342,7 +3338,7 @@ sre_traverse(PyObject *module, visitproc visit, void *arg) Py_VISIT(state->Match_Type); Py_VISIT(state->Scanner_Type); Py_VISIT(state->Template_Type); - Py_VISIT(state->compile_template); + Py_VISIT(state->re_module); return 0; } @@ -3356,7 +3352,7 @@ sre_clear(PyObject *module) Py_CLEAR(state->Match_Type); Py_CLEAR(state->Scanner_Type); Py_CLEAR(state->Template_Type); - Py_CLEAR(state->compile_template); + Py_CLEAR(state->re_module); return 0; } @@ -3409,6 +3405,12 @@ sre_exec(PyObject *m) goto error; } + PyObject *re = PyImport_ImportModule("re"); + if (re == NULL) { + goto error;
The code in compile_template() is a lazy import because re imports _sre, which is why you can't initialize compile_template in sre_exec. You can however import re in sre_exec and access it in compile_template(), do you want this one?
Ah, we had similar issue in asyncio as well with circular import, it was solved by delaying the import of C impl later, probably we can do something similar here like
cpython/Modules/_asynciomodule.c
Line 4435 in 247b50d
Although I generally prefer initializing module state in the exec function, I'm hesitant to delay or reorder the imports here and would rather just use the atomics.
The following seems to fix it:
From e1ef31d9417fe9247603fdc66f2acc5cecfbb074 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <kumaraditya@python.org> Date: Tue, 11 Feb 2025 15:55:55 +0000 Subject: [PATCH] fix re --- Lib/re/__init__.py | 16 +++++++++------- Modules/_sre/sre.c | 14 +++++++------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/Lib/re/__init__.py b/Lib/re/__init__.py index 7e8abbf6ffe..9773c643065 100644 --- a/Lib/re/__init__.py +++ b/Lib/re/__init__.py @@ -123,8 +123,16 @@ """ import enum -from . import _compiler, _parser import functools + +_MAXCACHE = 512 +@functools.lru_cache(_MAXCACHE) +def _compile_template(pattern, repl): + # internal: compile replacement pattern + return _sre.template(pattern, _parser.parse_template(repl, pattern)) + + +from . import _compiler, _parser import _sre @@ -323,7 +331,6 @@ def escape(pattern): # _cache uses the LRU policy which has better hit rate. _cache = {} # LRU _cache2 = {} # FIFO -_MAXCACHE = 512 _MAXCACHE2 = 256 assert _MAXCACHE2 < _MAXCACHE @@ -371,11 +378,6 @@ def _compile(pattern, flags): _cache2[key] = p return p -@functools.lru_cache(_MAXCACHE) -def _compile_template(pattern, repl): - # internal: compile replacement pattern - return _sre.template(pattern, _parser.parse_template(repl, pattern)) - # register myself for pickling import copyreg diff --git a/Modules/_sre/sre.c b/Modules/_sre/sre.c index 0d8d4843d33..a2b5f82d8b9 100644 --- a/Modules/_sre/sre.c +++ b/Modules/_sre/sre.c @@ -1168,13 +1168,7 @@ compile_template(_sremodulestate *module_state, { /* delegate to Python code */ PyObject *func = module_state->compile_template; - if (func == NULL) { - func = PyImport_ImportModuleAttrString("re", "_compile_template"); - if (func == NULL) { - return NULL; - } - Py_XSETREF(module_state->compile_template, func); - } + assert(func != NULL); PyObject *args[] = {(PyObject *)pattern, template}; PyObject *result = PyObject_Vectorcall(func, args, 2, NULL); @@ -3409,6 +3403,12 @@ sre_exec(PyObject *m) goto error; } + state->compile_template = PyImport_ImportModuleAttrString("re", "_compile_template"); + + if (state->compile_template == NULL) { + goto error; + } + return 0;
It seems that we need to just import _sre after defining _compile_template to avoid circular import.
It seems that we need to just import _sre after defining _compile_template to avoid circular import.
A bit fragile?
I'd be a lot more confident backporting the atomic change to 3.13 than reordering functions and imports in
re.- added a commit that references this issue
on Feb 11, 2025
Bug report
Concurrent accesses to
module_state->compile_templatein the free threaded buildcpython/Modules/_sre/sre.c
Lines 1169 to 1177 in 1feaecc
Linked PRs