[Python-checkins] GH-95909: Make `_PyArg_Parser` initialization thread safe (GH-95958)

[miss-islington]

https://github.com/python/cpython/commit/9b30b965f0c1da216397b495faef4d93ff7a5328
commit: 9b30b965f0c1da216397b495faef4d93ff7a5328
branch: main
author: Kumar Aditya <59607654+kumaraditya303 at users.noreply.github.com>
committer: miss-islington <31488909+miss-islington at users.noreply.github.com>
date: 2022-08-16T11:22:14-07:00
summary:

GH-95909: Make `_PyArg_Parser` initialization thread safe  (GH-95958)

files:
M Include/internal/pycore_runtime.h
M Python/getargs.c
M Python/pystate.c

diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h
index ae63ae74afa5..2c04ead45869 100644
--- a/Include/internal/pycore_runtime.h
+++ b/Include/internal/pycore_runtime.h
@@ -14,6 +14,9 @@ extern "C" {
 #include "pycore_interp.h"          // PyInterpreterState
 #include "pycore_unicodeobject.h"   // struct _Py_unicode_runtime_ids
 
+struct _getargs_runtime_state {
+   PyThread_type_lock mutex;
+};
 
 /* ceval state */
 
@@ -114,6 +117,7 @@ typedef struct pyruntimestate {
 
     struct _ceval_runtime_state ceval;
     struct _gilstate_runtime_state gilstate;
+    struct _getargs_runtime_state getargs;
 
     PyPreConfig preconfig;
 
diff --git a/Python/getargs.c b/Python/getargs.c
index 457dd99ce4a5..f0b84b8338dd 100644
--- a/Python/getargs.c
+++ b/Python/getargs.c
@@ -1974,15 +1974,10 @@ new_kwtuple(const char * const *keywords, int total, int pos)
 }
 
 static int
-parser_init(struct _PyArg_Parser *parser)
+_parser_init(struct _PyArg_Parser *parser)
 {
     const char * const *keywords = parser->keywords;
     assert(keywords != NULL);
-
-    if (parser->initialized) {
-        assert(parser->kwtuple != NULL);
-        return 1;
-    }
     assert(parser->pos == 0 &&
            (parser->format == NULL || parser->fname == NULL) &&
            parser->custom_msg == NULL &&
@@ -2035,6 +2030,28 @@ parser_init(struct _PyArg_Parser *parser)
     return 1;
 }
 
+static int
+parser_init(struct _PyArg_Parser *parser)
+{
+    // volatile as it can be modified by other threads
+    // and should not be optimized or reordered by compiler
+    if (*((volatile int *)&parser->initialized)) {
+        assert(parser->kwtuple != NULL);
+        return 1;
+    }
+    PyThread_acquire_lock(_PyRuntime.getargs.mutex, WAIT_LOCK);
+    // Check again if another thread initialized the parser
+    // while we were waiting for the lock.
+    if (*((volatile int *)&parser->initialized)) {
+        assert(parser->kwtuple != NULL);
+        PyThread_release_lock(_PyRuntime.getargs.mutex);
+        return 1;
+    }
+    int ret = _parser_init(parser);
+    PyThread_release_lock(_PyRuntime.getargs.mutex);
+    return ret;
+}
+
 static void
 parser_clear(struct _PyArg_Parser *parser)
 {
diff --git a/Python/pystate.c b/Python/pystate.c
index bcdb825a8629..642d680ba20b 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -57,7 +57,7 @@ _Py_COMP_DIAG_POP
 
 static int
 alloc_for_runtime(PyThread_type_lock *plock1, PyThread_type_lock *plock2,
-                  PyThread_type_lock *plock3)
+                  PyThread_type_lock *plock3, PyThread_type_lock *plock4)
 {
     /* Force default allocator, since _PyRuntimeState_Fini() must
        use the same allocator than this function. */
@@ -82,11 +82,20 @@ alloc_for_runtime(PyThread_type_lock *plock1, PyThread_type_lock *plock2,
         return -1;
     }
 
+    PyThread_type_lock lock4 = PyThread_allocate_lock();
+    if (lock4 == NULL) {
+        PyThread_free_lock(lock1);
+        PyThread_free_lock(lock2);
+        PyThread_free_lock(lock3);
+        return -1;
+    }
+
     PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc);
 
     *plock1 = lock1;
     *plock2 = lock2;
     *plock3 = lock3;
+    *plock4 = lock4;
     return 0;
 }
 
@@ -97,7 +106,8 @@ init_runtime(_PyRuntimeState *runtime,
              Py_ssize_t unicode_next_index,
              PyThread_type_lock unicode_ids_mutex,
              PyThread_type_lock interpreters_mutex,
-             PyThread_type_lock xidregistry_mutex)
+             PyThread_type_lock xidregistry_mutex,
+             PyThread_type_lock getargs_mutex)
 {
     if (runtime->_initialized) {
         Py_FatalError("runtime already initialized");
@@ -119,6 +129,8 @@ init_runtime(_PyRuntimeState *runtime,
 
     runtime->xidregistry.mutex = xidregistry_mutex;
 
+    runtime->getargs.mutex = getargs_mutex;
+
     // Set it to the ID of the main thread of the main interpreter.
     runtime->main_thread = PyThread_get_thread_ident();
 
@@ -141,8 +153,8 @@ _PyRuntimeState_Init(_PyRuntimeState *runtime)
     // is called multiple times.
     Py_ssize_t unicode_next_index = runtime->unicode_ids.next_index;
 
-    PyThread_type_lock lock1, lock2, lock3;
-    if (alloc_for_runtime(&lock1, &lock2, &lock3) != 0) {
+    PyThread_type_lock lock1, lock2, lock3, lock4;
+    if (alloc_for_runtime(&lock1, &lock2, &lock3, &lock4) != 0) {
         return _PyStatus_NO_MEMORY();
     }
 
@@ -152,7 +164,7 @@ _PyRuntimeState_Init(_PyRuntimeState *runtime)
         memcpy(runtime, &initial, sizeof(*runtime));
     }
     init_runtime(runtime, open_code_hook, open_code_userdata, audit_hook_head,
-                 unicode_next_index, lock1, lock2, lock3);
+                 unicode_next_index, lock1, lock2, lock3, lock4);
 
     return _PyStatus_OK();
 }
@@ -172,6 +184,7 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime)
     FREE_LOCK(runtime->interpreters.mutex);
     FREE_LOCK(runtime->xidregistry.mutex);
     FREE_LOCK(runtime->unicode_ids.lock);
+    FREE_LOCK(runtime->getargs.mutex);
 
 #undef FREE_LOCK
     PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc);
@@ -194,6 +207,7 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime)
     int reinit_interp = _PyThread_at_fork_reinit(&runtime->interpreters.mutex);
     int reinit_xidregistry = _PyThread_at_fork_reinit(&runtime->xidregistry.mutex);
     int reinit_unicode_ids = _PyThread_at_fork_reinit(&runtime->unicode_ids.lock);
+    int reinit_getargs = _PyThread_at_fork_reinit(&runtime->getargs.mutex);
 
     PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc);
 
@@ -204,7 +218,8 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime)
     if (reinit_interp < 0
         || reinit_main_id < 0
         || reinit_xidregistry < 0
-        || reinit_unicode_ids < 0)
+        || reinit_unicode_ids < 0
+        || reinit_getargs < 0)
     {
         return _PyStatus_ERR("Failed to reinitialize runtime locks");