Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-106168: Check allocated instead of size index bounds in PyList_SET_ITEM()#111480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 1 commit intopython:mainfromscoder:fix_pylist_set_item_check
Oct 30, 2023

Conversation

scoder
Copy link
Contributor

@scoderscoder commentedOct 30, 2023
edited by bedevere-appbot
Loading

Check the index bound assertions in PyList_SET_ITEM() against [0:allocated] instead of [0:size] to re-allow valid use cases that assign within the allocated area.

First increasing the size before setting a new value seems less safe and clean than allowing access to the allocated area and adjusting the size on successful updates.

…) against [0:allocated] instead of [0:size] to re-allow valid use cases that assign within the allocated area.
@scoder
Copy link
ContributorAuthor

I'm not sure what to make of the recommendation in the "what's new" paragraph. I specifically wouldn't recommend setting the size first, before setting the value. That seems incorrect and fragile. I'd remove it, but that'd leave us entirely without a recommendation.

@vstinner
Copy link
Member

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: callingrepr(list) can crash). See:

With this change,PyList_SetItem() andPyList_SET_ITEM() become inconsistent:PyList_SetItem() useslist->ob_size, whereasPyList_SET_ITEM() useslist->allocated.

PyList_SET_ITEM() is a strange beast: it doesn't DECREF the old value, it must only be used to initialize a list newly allocated by PyList_New(). ButPyList_New(size) setslist->ob_size tosize.

PyList_SetItem() is more regular: it calls DECREF on the old item.

Well, here the problem is not about designing a correct API, but to migrate Python 3.12 code to Python 3.13. Previously,PyList_SET_ITEM() could write safely in[0; allocated[ range, and in Python 3.13 it's limited to[0; ob_size[.

@vstinner
Copy link
Member

I'm not sure what to make of the recommendation in the "what's new" paragraph.

Since Python 3.13.0 final was not released, a Changelog entry is enough to document changes since 3.13 alpha1 (a NEWS.d entry added by the blurb tool).

@vstinnervstinner merged commit940ee96 intopython:mainOct 30, 2023
@vstinner
Copy link
Member

I merged your change without Changelog entry, I don't think that it's needed for such subtile change. Thanks.

@vstinner
Copy link
Member

I created issue#111489: [C API] Make _PyList_FromArraySteal() function public.

@scoderscoder deleted the fix_pylist_set_item_check branchNovember 1, 2023 09:06
@scoder
Copy link
ContributorAuthor

without Changelog entry, I don't think that it's needed for such subtile change

I was referring to the existing whats-new paragraph, which says "if the assertion fails, set the size first". That is no longer true.

@scoder
Copy link
ContributorAuthor

scoder commentedNov 1, 2023
edited
Loading

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: callingrepr(list) can crash)

That's why I prefernot requiring users to change the sizebefore they set the value. Doing that would bring the list in an invalid state.

With this change,PyList_SetItem() andPyList_SET_ITEM() become inconsistent:PyList_SetItem() useslist->ob_size, whereasPyList_SET_ITEM() useslist->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 thatPyList_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
Copy link
Member

I was referring to the existing whats-new paragraph, which says "if the assertion fails, set the size first". That is no longer true.

Ah oops, I tried to find a What's New entry, but I failed to find it, and so made the assumption that I didn't document the change. It should be updated, you're right.

@vstinner
Copy link
Member

Ah oops, I tried to find a What's New entry, but I failed to find it, and so made the assumption that I didn't document the change. It should be updated, you're right.

I wrote PR#111618 to update What's New in Python 3.13.

FullteaR pushed a commit to FullteaR/cpython that referenced this pull requestNov 3, 2023
…st_SET_ITEM() (python#111480)Check the index bound assertions in PyList_SET_ITEM() against [0:allocated] instead of [0:size] to re-allow valid use cases that assign within the allocated area.
aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
…st_SET_ITEM() (python#111480)Check the index bound assertions in PyList_SET_ITEM() against [0:allocated] instead of [0:size] to re-allow valid use cases that assign within the allocated area.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
…st_SET_ITEM() (python#111480)Check the index bound assertions in PyList_SET_ITEM() against [0:allocated] instead of [0:size] to re-allow valid use cases that assign within the allocated area.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnerAwaiting requested review from vstinner

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@scoder@vstinner

[8]ページ先頭

©2009-2025 Movatter.jp