From 208f247bdc57e1e2282538a59427edf66e26042e Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 22 Apr 2025 16:52:20 +0200 Subject: [PATCH 1/7] gh-132713: Fix typing.Union[index] race condition Use a critical section in union_getitem() to initialize the 'parameters' member. --- Objects/unionobject.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index 0b7d4c72bffb97..00ccedebb1623c 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -328,7 +328,12 @@ union_getitem(PyObject *self, PyObject *item) unionobject *alias = (unionobject *)self; // Populate __parameters__ if needed. if (alias->parameters == NULL) { - alias->parameters = _Py_make_parameters(alias->args); + Py_BEGIN_CRITICAL_SECTION(alias); + if (alias->parameters == NULL) { + alias->parameters = _Py_make_parameters(alias->args); + } + Py_END_CRITICAL_SECTION(); + if (alias->parameters == NULL) { return NULL; } From d81ec9b042be619c55706b77c264cd57c5f56020 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 22 Apr 2025 17:19:56 +0200 Subject: [PATCH 2/7] Add comment --- Objects/unionobject.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index 00ccedebb1623c..2b0bc06de8f03b 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -329,6 +329,8 @@ union_getitem(PyObject *self, PyObject *item) // Populate __parameters__ if needed. if (alias->parameters == NULL) { Py_BEGIN_CRITICAL_SECTION(alias); + // Need to check again once we got the lock, another thread may have + // initialized it while we were waiting for the lock. if (alias->parameters == NULL) { alias->parameters = _Py_make_parameters(alias->args); } From d93396a177296fe55ff67fbf525b4c607fdddfcf Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 22 Apr 2025 17:24:11 +0200 Subject: [PATCH 3/7] Add comment about the micro-optimization --- Objects/unionobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index 2b0bc06de8f03b..2f8b77974665ae 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -327,6 +327,7 @@ union_getitem(PyObject *self, PyObject *item) { unionobject *alias = (unionobject *)self; // Populate __parameters__ if needed. + // Avoid the critical section if it's already initialized. if (alias->parameters == NULL) { Py_BEGIN_CRITICAL_SECTION(alias); // Need to check again once we got the lock, another thread may have From f12f9055167a09701a964406c98bdaa1ee202b19 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 22 Apr 2025 21:47:44 +0200 Subject: [PATCH 4/7] Fix also union_parameters() Add union_init_parameters() helper function. --- Objects/unionobject.c | 44 ++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index 2f8b77974665ae..c39964cb96b25d 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -322,24 +322,37 @@ static PyMemberDef union_members[] = { {0} }; -static PyObject * -union_getitem(PyObject *self, PyObject *item) +// Populate __parameters__ if needed. +static int +union_init_parameters(unionobject *alias) { - unionobject *alias = (unionobject *)self; - // Populate __parameters__ if needed. // Avoid the critical section if it's already initialized. + if (alias->parameters != NULL) { + return 0; + } + + int result = 0; + Py_BEGIN_CRITICAL_SECTION(alias); + // Need to check again once we got the lock, another thread may have + // initialized it while we were waiting for the lock. if (alias->parameters == NULL) { - Py_BEGIN_CRITICAL_SECTION(alias); - // Need to check again once we got the lock, another thread may have - // initialized it while we were waiting for the lock. + alias->parameters = _Py_make_parameters(alias->args); if (alias->parameters == NULL) { - alias->parameters = _Py_make_parameters(alias->args); + result = -1; } - Py_END_CRITICAL_SECTION(); + } + Py_END_CRITICAL_SECTION(); - if (alias->parameters == NULL) { - return NULL; - } + return result; +} + +static PyObject * +union_getitem(PyObject *self, PyObject *item) +{ + unionobject *alias = (unionobject *)self; + + if (union_init_parameters(alias) < 0) { + return NULL; } PyObject *newargs = _Py_subs_parameters(self, alias->args, alias->parameters, item); @@ -360,11 +373,8 @@ static PyObject * union_parameters(PyObject *self, void *Py_UNUSED(unused)) { unionobject *alias = (unionobject *)self; - if (alias->parameters == NULL) { - alias->parameters = _Py_make_parameters(alias->args); - if (alias->parameters == NULL) { - return NULL; - } + if (union_init_parameters(alias) < 0) { + return NULL; } return Py_NewRef(alias->parameters); } From a7b74a36b9a339d11bd103aa94e963234aec5a5c Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 22 Apr 2025 21:58:08 +0200 Subject: [PATCH 5/7] Use FT_ATOMIC to load/store the pointer --- Objects/unionobject.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index c39964cb96b25d..a259626461040c 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -327,7 +327,7 @@ static int union_init_parameters(unionobject *alias) { // Avoid the critical section if it's already initialized. - if (alias->parameters != NULL) { + if (FT_ATOMIC_LOAD_PTR_RELAXED(alias->parameters) != NULL) { return 0; } @@ -336,7 +336,8 @@ union_init_parameters(unionobject *alias) // Need to check again once we got the lock, another thread may have // initialized it while we were waiting for the lock. if (alias->parameters == NULL) { - alias->parameters = _Py_make_parameters(alias->args); + FT_ATOMIC_STORE_PTR(alias->parameters, + _Py_make_parameters(alias->args)); if (alias->parameters == NULL) { result = -1; } From 27da04dec50994f2a2a2b57b258d4c8d9d5ed414 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 23 Apr 2025 09:12:27 +0200 Subject: [PATCH 6/7] Remove the optimization --- Objects/unionobject.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index a259626461040c..d66f5f7f479840 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -326,24 +326,15 @@ static PyMemberDef union_members[] = { static int union_init_parameters(unionobject *alias) { - // Avoid the critical section if it's already initialized. - if (FT_ATOMIC_LOAD_PTR_RELAXED(alias->parameters) != NULL) { - return 0; - } - int result = 0; Py_BEGIN_CRITICAL_SECTION(alias); - // Need to check again once we got the lock, another thread may have - // initialized it while we were waiting for the lock. if (alias->parameters == NULL) { - FT_ATOMIC_STORE_PTR(alias->parameters, - _Py_make_parameters(alias->args)); + alias->parameters = _Py_make_parameters(alias->args); if (alias->parameters == NULL) { result = -1; } } Py_END_CRITICAL_SECTION(); - return result; } From 26c9a3298484fcebcf636b10ac5803cdf08be270 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 23 Apr 2025 09:15:23 +0200 Subject: [PATCH 7/7] Remove empty line --- Objects/unionobject.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Objects/unionobject.c b/Objects/unionobject.c index d66f5f7f479840..66435924b6c6c3 100644 --- a/Objects/unionobject.c +++ b/Objects/unionobject.c @@ -342,7 +342,6 @@ static PyObject * union_getitem(PyObject *self, PyObject *item) { unionobject *alias = (unionobject *)self; - if (union_init_parameters(alias) < 0) { return NULL; }