GeezerVison is showing github.com — C API: Check in PyTuple_SET_ITEM() and PyList_SET_ITEM() · Issue #106168 · python/cpython
C API: Check in PyTuple_SET_ITEM() and PyList_SET_ITEM() #106168DescriptionThe PyTuple_SET_ITEM() and PyList_SET_ITEM() functions don't check that the index is valid. It should be checked with an assertion.
Linked PRs
Activityscoder commented on Oct 29, 2023
The PyTuple_SET_ITEM() and PyList_SET_ITEM() functions don't check that the index is valid. It should be checked with an assertion.
I oppose this statement. Their documentation says that both allow unchecked writing into the list/tuple. Adding these checks breaks their contract. scoder commented on 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. added a commit that references this issue on Oct 30, 2023gh-106168: Check allocated instead of size index bounds in PyList_SET… scoder commented on 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:
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. vstinner commented on 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 👍
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|