Skip to content

Commit 4936fa9

Browse files
vstinnerchgnrdv
vstinner
and
chgnrdv
authored
[3.12] gh-108987: Fix _thread.start_new_thread() race condition (#109135) (#110342)
* gh-108987: Fix _thread.start_new_thread() race condition (#109135) Fix _thread.start_new_thread() race condition. If a thread is created during Python finalization, the newly spawned thread now exits immediately instead of trying to access freed memory and lead to a crash. thread_run() calls PyEval_AcquireThread() which checks if the thread must exit. The problem was that tstate was dereferenced earlier in _PyThreadState_Bind() which leads to a crash most of the time. Move _PyThreadState_CheckConsistency() from thread_run() to _PyThreadState_Bind(). (cherry picked from commit 517cd82) * gh-109795: `_thread.start_new_thread`: allocate thread bootstate using raw memory allocator (#109808) (cherry picked from commit 1b8f236) --------- Co-authored-by: Radislav Chugunov <52372310+chgnrdv@users.noreply.github.com>
1 parent 1d87465 commit 4936fa9

5 files changed

Lines changed: 75 additions & 44 deletions

File tree

‎Include/internal/pycore_pystate.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@ PyAPI_DATA(PyThreadState *) _PyThreadState_GetCurrent(void);
7272
extern int _PyThreadState_CheckConsistency(PyThreadState *tstate);
7373
#endif
7474

75+
extern int _PyThreadState_MustExit(PyThreadState *tstate);
76+
7577
/* Get the current Python thread state.
7678
7779
This function is unsafe: it does not check for error and it can return NULL.

‎Misc/NEWS.d/next/Library/2023-09-08-12-09-55.gh-issue-108987.x5AIG8.rst‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix :func:`_thread.start_new_thread` race condition. If a thread is created
2+
during Python finalization, the newly spawned thread now exits immediately
3+
instead of trying to access freed memory and lead to a crash. Patch by
4+
Victor Stinner.

‎Modules/_threadmodule.c‎

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,22 +1063,22 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref)
10631063
/* Module functions */
10641064

10651065
struct bootstate {
1066-
PyInterpreterState *interp;
1066+
PyThreadState *tstate;
10671067
PyObject *func;
10681068
PyObject *args;
10691069
PyObject *kwargs;
1070-
PyThreadState *tstate;
1071-
_PyRuntimeState *runtime;
10721070
};
10731071

10741072

10751073
static void
1076-
thread_bootstate_free(struct bootstate *boot)
1074+
thread_bootstate_free(struct bootstate *boot, int decref)
10771075
{
1078-
Py_DECREF(boot->func);
1079-
Py_DECREF(boot->args);
1080-
Py_XDECREF(boot->kwargs);
1081-
PyMem_Free(boot);
1076+
if (decref) {
1077+
Py_DECREF(boot->func);
1078+
Py_DECREF(boot->args);
1079+
Py_XDECREF(boot->kwargs);
1080+
}
1081+
PyMem_RawFree(boot);
10821082
}
10831083

10841084

@@ -1088,9 +1088,24 @@ thread_run(void *boot_raw)
10881088
struct bootstate *boot = (struct bootstate *) boot_raw;
10891089
PyThreadState *tstate = boot->tstate;
10901090

1091-
// gh-104690: If Python is being finalized and PyInterpreterState_Delete()
1092-
// was called, tstate becomes a dangling pointer.
1093-
assert(_PyThreadState_CheckConsistency(tstate));
1091+
// gh-108987: If _thread.start_new_thread() is called before or while
1092+
// Python is being finalized, thread_run() can called *after*.
1093+
// _PyRuntimeState_SetFinalizing() is called. At this point, all Python
1094+
// threads must exit, except of the thread calling Py_Finalize() whch holds
1095+
// the GIL and must not exit.
1096+
//
1097+
// At this stage, tstate can be a dangling pointer (point to freed memory),
1098+
// it's ok to call _PyThreadState_MustExit() with a dangling pointer.
1099+
if (_PyThreadState_MustExit(tstate)) {
1100+
// Don't call PyThreadState_Clear() nor _PyThreadState_DeleteCurrent().
1101+
// These functions are called on tstate indirectly by Py_Finalize()
1102+
// which calls _PyInterpreterState_Clear().
1103+
//
1104+
// Py_DECREF() cannot be called because the GIL is not held: leak
1105+
// references on purpose. Python is being finalized anyway.
1106+
thread_bootstate_free(boot, 0);
1107+
goto exit;
1108+
}
10941109

10951110
_PyThreadState_Bind(tstate);
10961111
PyEval_AcquireThread(tstate);
@@ -1109,14 +1124,17 @@ thread_run(void *boot_raw)
11091124
Py_DECREF(res);
11101125
}
11111126

1112-
thread_bootstate_free(boot);
1127+
thread_bootstate_free(boot, 1);
1128+
11131129
tstate->interp->threads.count--;
11141130
PyThreadState_Clear(tstate);
11151131
_PyThreadState_DeleteCurrent(tstate);
11161132

1133+
exit:
11171134
// bpo-44434: Don't call explicitly PyThread_exit_thread(). On Linux with
11181135
// the glibc, pthread_exit() can abort the whole process if dlopen() fails
11191136
// to open the libgcc_s.so library (ex: EMFILE error).
1137+
return;
11201138
}
11211139

11221140
static PyObject *
@@ -1140,7 +1158,6 @@ and False otherwise.\n");
11401158
static PyObject *
11411159
thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs)
11421160
{
1143-
_PyRuntimeState *runtime = &_PyRuntime;
11441161
PyObject *func, *args, *kwargs = NULL;
11451162

11461163
if (!PyArg_UnpackTuple(fargs, "start_new_thread", 2, 3,
@@ -1179,20 +1196,21 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs)
11791196
return NULL;
11801197
}
11811198

1182-
struct bootstate *boot = PyMem_NEW(struct bootstate, 1);
1199+
// gh-109795: Use PyMem_RawMalloc() instead of PyMem_Malloc(),
1200+
// because it should be possible to call thread_bootstate_free()
1201+
// without holding the GIL.
1202+
struct bootstate *boot = PyMem_RawMalloc(sizeof(struct bootstate));
11831203
if (boot == NULL) {
11841204
return PyErr_NoMemory();
11851205
}
1186-
boot->interp = _PyInterpreterState_GET();
1187-
boot->tstate = _PyThreadState_New(boot->interp);
1206+
boot->tstate = _PyThreadState_New(interp);
11881207
if (boot->tstate == NULL) {
1189-
PyMem_Free(boot);
1208+
PyMem_RawFree(boot);
11901209
if (!PyErr_Occurred()) {
11911210
return PyErr_NoMemory();
11921211
}
11931212
return NULL;
11941213
}
1195-
boot->runtime = runtime;
11961214
boot->func = Py_NewRef(func);
11971215
boot->args = Py_NewRef(args);
11981216
boot->kwargs = Py_XNewRef(kwargs);
@@ -1201,7 +1219,7 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs)
12011219
if (ident == PYTHREAD_INVALID_THREAD_ID) {
12021220
PyErr_SetString(ThreadError, "can't start new thread");
12031221
PyThreadState_Clear(boot->tstate);
1204-
thread_bootstate_free(boot);
1222+
thread_bootstate_free(boot, 1);
12051223
return NULL;
12061224
}
12071225
return PyLong_FromUnsignedLong(ident);

‎Python/ceval_gil.c‎

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -328,28 +328,6 @@ drop_gil(struct _ceval_state *ceval, PyThreadState *tstate)
328328
}
329329

330330

331-
/* Check if a Python thread must exit immediately, rather than taking the GIL
332-
if Py_Finalize() has been called.
333-
334-
When this function is called by a daemon thread after Py_Finalize() has been
335-
called, the GIL does no longer exist.
336-
337-
tstate must be non-NULL. */
338-
static inline int
339-
tstate_must_exit(PyThreadState *tstate)
340-
{
341-
/* bpo-39877: Access _PyRuntime directly rather than using
342-
tstate->interp->runtime to support calls from Python daemon threads.
343-
After Py_Finalize() has been called, tstate can be a dangling pointer:
344-
point to PyThreadState freed memory. */
345-
PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime);
346-
if (finalizing == NULL) {
347-
finalizing = _PyInterpreterState_GetFinalizing(tstate->interp);
348-
}
349-
return (finalizing != NULL && finalizing != tstate);
350-
}
351-
352-
353331
/* Take the GIL.
354332
355333
The function saves errno at entry and restores its value at exit.
@@ -365,7 +343,7 @@ take_gil(PyThreadState *tstate)
365343
// XXX It may be more correct to check tstate->_status.finalizing.
366344
// XXX assert(!tstate->_status.cleared);
367345

368-
if (tstate_must_exit(tstate)) {
346+
if (_PyThreadState_MustExit(tstate)) {
369347
/* bpo-39877: If Py_Finalize() has been called and tstate is not the
370348
thread which called Py_Finalize(), exit immediately the thread.
371349
@@ -403,7 +381,7 @@ take_gil(PyThreadState *tstate)
403381
_Py_atomic_load_relaxed(&gil->locked) &&
404382
gil->switch_number == saved_switchnum)
405383
{
406-
if (tstate_must_exit(tstate)) {
384+
if (_PyThreadState_MustExit(tstate)) {
407385
MUTEX_UNLOCK(gil->mutex);
408386
// gh-96387: If the loop requested a drop request in a previous
409387
// iteration, reset the request. Otherwise, drop_gil() can
@@ -443,7 +421,7 @@ take_gil(PyThreadState *tstate)
443421
MUTEX_UNLOCK(gil->switch_mutex);
444422
#endif
445423

446-
if (tstate_must_exit(tstate)) {
424+
if (_PyThreadState_MustExit(tstate)) {
447425
/* bpo-36475: If Py_Finalize() has been called and tstate is not
448426
the thread which called Py_Finalize(), exit immediately the
449427
thread.

‎Python/pystate.c‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1867,6 +1867,10 @@ PyThreadState_Swap(PyThreadState *newts)
18671867
void
18681868
_PyThreadState_Bind(PyThreadState *tstate)
18691869
{
1870+
// gh-104690: If Python is being finalized and PyInterpreterState_Delete()
1871+
// was called, tstate becomes a dangling pointer.
1872+
assert(_PyThreadState_CheckConsistency(tstate));
1873+
18701874
bind_tstate(tstate);
18711875
// This makes sure there's a gilstate tstate bound
18721876
// as soon as possible.
@@ -2866,6 +2870,31 @@ _PyThreadState_CheckConsistency(PyThreadState *tstate)
28662870
#endif
28672871

28682872

2873+
// Check if a Python thread must exit immediately, rather than taking the GIL
2874+
// if Py_Finalize() has been called.
2875+
//
2876+
// When this function is called by a daemon thread after Py_Finalize() has been
2877+
// called, the GIL does no longer exist.
2878+
//
2879+
// tstate can be a dangling pointer (point to freed memory): only tstate value
2880+
// is used, the pointer is not deferenced.
2881+
//
2882+
// tstate must be non-NULL.
2883+
int
2884+
_PyThreadState_MustExit(PyThreadState *tstate)
2885+
{
2886+
/* bpo-39877: Access _PyRuntime directly rather than using
2887+
tstate->interp->runtime to support calls from Python daemon threads.
2888+
After Py_Finalize() has been called, tstate can be a dangling pointer:
2889+
point to PyThreadState freed memory. */
2890+
PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime);
2891+
if (finalizing == NULL) {
2892+
finalizing = _PyInterpreterState_GetFinalizing(tstate->interp);
2893+
}
2894+
return (finalizing != NULL && finalizing != tstate);
2895+
}
2896+
2897+
28692898
#ifdef __cplusplus
28702899
}
28712900
#endif

0 commit comments

Comments
 (0)