| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Concurrent calls to sys.set_int_max_str_digits() in free-threaded builds could double-free the same sys.flags tuple item because sys_set_flag() updated the slot without synchronization. Protect sys.flags updates with a mutex in free-threaded builds and hold the same lock across the flag and interpreter int_max_str_digits state updates so sys.get_int_max_str_digits() stays consistent with sys.flags.
|
Please do not force push. To contribute cleanly, please read https://devguide.python.org/getting-started/pull-request-lifecycle/#pullrequest. |
Sorry, something went wrong.
|
This doesn't guard the read though. And the read through sys.flags is probably difficult to guard because it's an "immutable" object and so shouldn't be changing once it's visible |
Sorry, something went wrong.
Good point — you're right that the read path through sys.flags isn't A few options I see:
Option 1 looks cleanest and matches the "immutable" intent, but it's |
Sorry, something went wrong.
|
This conversation seems to go against our AI policy. Human interaction is necessary, don't let your agents in automated mode. Please read https://devguide.python.org/getting-started/ai-tools/#guidelines-for-using-ai-tools. |
Sorry, something went wrong.
Sorry — I used an AI assistant to draft PR replies and posted them On the technical side: I understand the mutex doesn't protect reads |
Sorry, something went wrong.
There was a problem hiding this comment.
I think that exposing int_max_str_digits in the sys.flags was a mistake. This is an immutable named tuple for command-line flags.
But sys.flags.int_max_str_digits does not fit in this picture: it's for runtime settings, which can be changed by sys.set_int_max_str_digits().
I think this bug should be fixed by changing meaning of the sys.flags.int_max_str_digits. It will keep immutable value, provided as command-line option. We have a different API to access runtime settings, i.e. sys.get_int_max_str_digits(). CC @gpshead
Sorry, something went wrong.
|
PyConfig_Set() per PEP 741 in 3.14 is a public API that mutates various other sys.flags values at runtime. It is no longer correct to claim that it is a tuple of command line flags that never changes after the interpreter starts. I do not think changing what happens with int_max_str_digits would be right at this point. |
Sorry, something went wrong.
There was a problem hiding this comment.
overall I don't think this PR is the right way to accomplish this. we don't need a static global mutex. use Py_BEGIN_CRITICAL_SECTION on the flags object instead.
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
I don't think that works because there's no thread-safety guards on any of the reads from the flags object (because it's a tuple-like object so considered immutable)
I do agree with this though |
Sorry, something went wrong.
Indeed. Then struct sequence doesn't look to be the right data structure for this sys attribute anymore. Or, I suspect, we will reinvent the wheel (dict?).
Maybe use a dict subclass (to make backward-compatible __getattr__) instead of the named tuple? Mutating of the struct sequence breaks the contract in docs:
CC @vstinner (as PEP author) Edit: So far, option (3) from proposed above seems to be an alternative to this. |
Sorry, something went wrong.
|
its perfectly fine for it to be a structsequence. we should probably just do as tuples do and replace the instance with a new structsequence instance upon each mutation (a copy with one element changed) instead of trying to treat that as mutable or piling even more odd "some elements can change via getitem/getattr implementations returning new info" shapes on top of it. the design mistake was ever using tuples for anything in the 1990s followed by recognizing that namedtuples weren't a solution to their ills in the 2000s. IIRC that's how structsequence came into being? an odd duck shaped thing that floats to preserve legacy API expectations while letting us add new .field_names. the joys of the standard library and specifically old things like sys. |
Sorry, something went wrong.
| #include "pycore_initconfig.h" // _PyStatus_EXCEPTION() | ||
| #include "pycore_interpframe.h" // _PyFrame_GetFirstComplete() | ||
| #ifdef Py_GIL_DISABLED | ||
| # include "pycore_lock.h" // PyMutex_Lock() |
There was a problem hiding this comment.
PyMutex_Lock() is part of Python.h. There is no need to include the internal pycore_lock.h header.
Sorry, something went wrong.
| assert(pos >= 0 && pos < (Py_ssize_t)(Py_ARRAY_LENGTH(flags_fields) - 1)); | ||
|
|
||
| PyObject *old_value = PyStructSequence_GET_ITEM(flags, pos); | ||
| *p_old_value = PyStructSequence_GET_ITEM(flags, pos); |
There was a problem hiding this comment.
Why do you call Py_XDECREF(old_value); in the caller? It seems same to keep Py_XDECREF(old_value); in this function, no?
Sorry, something went wrong.
| }; | ||
|
|
||
| #ifdef Py_GIL_DISABLED | ||
| static PyMutex sys_flags_mutex; |
There was a problem hiding this comment.
Please add a comment to explain the purpose of this lock. IMO it should only be used when modifying sys.flags. It's not needed to acquire this lock to get the sys.flags attribute.
Sorry, something went wrong.
|
|
||
|
|
||
| static int | ||
| _PySys_SetFlagInt(Py_ssize_t pos, int value) |
There was a problem hiding this comment.
You can keep this function, it doesn't hurt.
Sorry, something went wrong.
| } | ||
|
|
||
| #ifdef Py_GIL_DISABLED | ||
| PyMutex_Lock(&sys_flags_mutex); |
There was a problem hiding this comment.
I don't think that you need to acquire the lock to get sys.flags. You only need the lock around the code changing sys.flags members.
Sorry, something went wrong.
|
You should also update _PySys_UpdateConfig() to acquire the lock to call set_flags_from_config(). make_flags() doesn't need to be modified, since sys.flags doesn't exist when it's called. |
Sorry, something went wrong.
Would you mind to elaborate why a lock would also be needed to read a sys.flags flag? sys.flags is a structseq instance which inherits from tuple. Getting a tuple item doesn't use any lock: static PyObject *
tuple_item(PyObject *op, Py_ssize_t i)
{
PyTupleObject *a = _PyTuple_CAST(op);
if (i < 0 || i >= Py_SIZE(a)) {
PyErr_SetString(PyExc_IndexError, "tuple index out of range");
return NULL;
}
return Py_NewRef(a->ob_item[i]);
}
Ah, that sounds like a reasonable solution. |
Sorry, something went wrong.
|
I wrote PR gh-151402 to implement the other option: PyConfig_Set() and sys.set_int_max_str_digits() now replace sys.flags, instead of modifying sys.flags in-place. |
Sorry, something went wrong.
|
Thanks everyone for the review. I see the consensus is to replace sys.flags on mutation rather than locking in-place updates — and Victor's PR #151402 implements that. I'll close this PR. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes #151218
Concurrent calls to sys.set_int_max_str_digits() in free-threaded builds
could double-free the same sys.flags tuple item because sys_set_flag()
updated the slot without synchronization.
Protect sys.flags updates with a mutex in free-threaded builds, and hold
the same lock across flag and interpreter int_max_str_digits state updates
so sys.get_int_max_str_digits() stays consistent with sys.flags. Add a
concurrent stress regression test in test_sys.py.