GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-106168: Check allocated instead of size index bounds in PyList_SET_ITEM() by scoder · Pull Request #111480 · python/cpython

Conversation

@scoder

@scoder scoder commented Oct 30, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: C API: Check in PyTuple_SET_ITEM() and PyList_SET_ITEM() #106168

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@scoder
pythongh-106168: Check the index bound assertions in PyList_SET_ITEM(…
77681ba
…) against [0:allocated] instead of [0:size] to re-allow valid use cases that assign within the allocated area.
@scoder
scoder requested a review from vstinner October 30, 2023 08:11
@bedevere-app bedevere-app Bot mentioned this pull request Oct 30, 2023
C API: Check in PyTuple_SET_ITEM() and PyList_SET_ITEM() #106168
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 30, 2023
@scoder

scoder commented Oct 30, 2023

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 30, 2023

Copy link
Copy Markdown
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: calling repr(list) can crash). See:

  • Disallow creation of incomplete/inconsistent objects capi-workgroup/problems#56
  • gh-107137: Add _PyTupleBuilder API to the internal C API #107139
  • Avoid creating incomplete/invalid objects capi-workgroup/api-evolution#36

With this change, PyList_SetItem() and PyList_SET_ITEM() become inconsistent: PyList_SetItem() uses list->ob_size, whereas PyList_SET_ITEM() uses list->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(). But PyList_New(size) sets list->ob_size to size.

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[.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner added the skip news label Oct 30, 2023
@vstinner

vstinner commented Oct 30, 2023

Copy link
Copy Markdown
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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit 940ee96 into python:main Oct 30, 2023
@bedevere-app bedevere-app Bot removed the awaiting core review label Oct 30, 2023
@vstinner

vstinner commented Oct 30, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Oct 30, 2023
Avoid creating incomplete/invalid objects capi-workgroup/api-evolution#36
Open
@vstinner

vstinner commented Oct 30, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@scoder
scoder deleted the fix_pylist_set_item_check branch November 1, 2023 09:06
@scoder

scoder commented Nov 1, 2023

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@scoder

scoder commented Nov 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 1, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 1, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
@scoder @FullteaR
pythongh-106168: Check allocated instead of size index bounds in PyLi…
0582d79
…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 request Feb 11, 2024
@scoder @aisk
pythongh-106168: Check allocated instead of size index bounds in PyLi…
6f86bcd
…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 request Sep 2, 2024
@scoder @Glyphack
pythongh-106168: Check allocated instead of size index bounds in PyLi…
0f86605
…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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@scoder @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.