You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
C API: Check in PyTuple_SET_ITEM() and PyList_SET_ITEM() #106168
If you insist on adding such a check, you can assert that the index is within the currently allocated array bounds. Writing outside of that range is definitely incorrect. However, writing beyond the current size may not be.
I dislike PyList_New() + PyList_SET_ITEM() API since the list is immediately tracked by the GC and so calling gc.get_objects() can expose an invalid list object (ex: calling repr(list) can crash)
That's why I prefer not requiring users to change the size before they set the value. Doing that would bring the list in an invalid state.
With this change, PyList_SetItem() and PyList_SET_ITEM() become inconsistent: PyList_SetItem() uses list->ob_size, whereas PyList_SET_ITEM() uses list->allocated.
I don't see an inconsistency here. They serve different needs, that's why we have two different functions. As you write, PyList_SetItem() decrefs the current value and replaces it with a new one. That can only safely be done within [0:size]. PyList_SET_ITEM() writes a value to an array position that does not yet have a value. That can only safely be done within [0:allocated]. Different needs, different functions, different boundary conditions.
You could rather argue that PyList_SET_ITEM() is only really valid within [size:allocated]. Within [0:size], it would overwrite existing, owned values. However, enforcing that would probably break even more code that might just look slightly brittle but is actually working perfectly fine (because it does what you suggested, it updates the size immediately before setting the value).
Cython uses the _SET_ITEM() functions to initialise freshly created lists and tuples, but it also calls PyList_SET_ITEM() in a fast, inlined PyList_Append() implementation: https://github.com/cython/cython/blob/master/Cython/Utility/Optimize.c#L29-L77
We use this for list comprehensions, for example, where we control the list internally. As long as the next item fits into the already allocated array, we just put it there and increase the size.
I changed PyList the same way than PyTuple, but as you described, PyList is different and requires special treatement. I'm fine with #111480 that's why I merged it 👍
Activity
pythongh-106168: PyTuple_SET_ITEM() now checks the index
pythongh-106168: PyTuple_SET_ITEM() now checks the index
pythongh-106168: PyTuple_SET_ITEM() now checks the index
pythongh-106168: PyTuple_SET_ITEM() now checks the index
gh-106168: PyTuple_SET_ITEM() now checks the index (#106164)
scoder commentedon Oct 29, 2023
I oppose this statement. Their documentation says that both allow unchecked writing into the list/tuple. Adding these checks breaks their contract.
scoder commentedon Oct 29, 2023
If you insist on adding such a check, you can assert that the index is within the currently allocated array bounds. Writing outside of that range is definitely incorrect. However, writing beyond the current size may not be.
Work around a change in CPython 3.13a1 that prevents calling PyList_S…
pythongh-106168: Check the index bound assertions in PyList_SET_ITEM(…
gh-106168: Check allocated instead of size index bounds in PyList_SET…
scoder commentedon Nov 1, 2023
[Copying my comment from the PR here as IMHO it describes the situation quite well and thus belongs rather into the ticket discussion.]
@vstinner wrote:
That's why I prefer not requiring users to change the size before they set the value. Doing that would bring the list in an invalid state.
I don't see an inconsistency here. They serve different needs, that's why we have two different functions. As you write,
PyList_SetItem()decrefs the current value and replaces it with a new one. That can only safely be done within [0:size].PyList_SET_ITEM()writes a value to an array position that does not yet have a value. That can only safely be done within [0:allocated]. Different needs, different functions, different boundary conditions.You could rather argue that
PyList_SET_ITEM()is only really valid within [size:allocated]. Within [0:size], it would overwrite existing, owned values. However, enforcing that would probably break even more code that might just look slightly brittle but is actually working perfectly fine (because it does what you suggested, it updates the size immediately before setting the value).Cython uses the
_SET_ITEM()functions to initialise freshly created lists and tuples, but it also callsPyList_SET_ITEM()in a fast, inlinedPyList_Append()implementation:https://github.com/cython/cython/blob/master/Cython/Utility/Optimize.c#L29-L77
We use this for list comprehensions, for example, where we control the list internally. As long as the next item fits into the already allocated array, we just put it there and increase the size.
vstinner commentedon Nov 1, 2023
I changed PyList the same way than PyTuple, but as you described, PyList is different and requires special treatement. I'm fine with #111480 that's why I merged it 👍
pythongh-106168: Update PyList_SET_ITEM() What's New doc
pythongh-106168: Update PyList_SET_ITEM() What's New doc
gh-106168: Update PyList_SET_ITEM() What's New doc (#111618)
pythongh-106168: Check allocated instead of size index bounds in PyLi…
pythongh-106168: Update PyList_SET_ITEM() What's New doc (python#111618)
pythongh-106168: Update the size only after setting the item, to avoi…
gh-106168: Revert the "size before item" setting (#111683)
pythongh-106168: Check allocated instead of size index bounds in PyLi…
pythongh-106168: Update PyList_SET_ITEM() What's New doc (python#111618)
pythongh-106168: Revert the "size before item" setting (python#111683)
pythongh-106168: Check allocated instead of size index bounds in PyLi…
pythongh-106168: Update PyList_SET_ITEM() What's New doc (python#111618)
pythongh-106168: Revert the "size before item" setting (python#111683)