Skip to content

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating __index__ methods - #132379

Merged
picnixz merged 9 commits into
python:mainfrom
bast0006:bast0006/gh-91153
Jul 12, 2025
Merged

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating __index__ methods#132379
picnixz merged 9 commits into
python:mainfrom
bast0006:bast0006/gh-91153

Conversation

@bast0006

@bast0006bast0006 commented Apr 10, 2025•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

bytearray's __setitem__ implementation currently grabs a reference to its internal buffer before calling _getbyvalue to determine the index that needs assignment. _getbyvalue can call into arbitrary python code via __index__ dunders, which could alter the internal buffer and leave said reference dangling.

A prior fix for this issue ensures that bounds checking occurs after _getbyvalue is called. However, python code is capable of resizing the bytearray, resulting in limited but still broken behavior.

This patch ensures that the reference to the internal buffer is fetched only after _getbyvalue is called to prevent it from being held while any python code is run.

@python-cla-bot

python-cla-botBot commented Apr 10, 2025•
edited
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app

bedevere-appBot commented Apr 10, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-appbedevere-appBot mentioned this pull request Apr 10, 2025
Closed
@bast0006

bast0006 commented Apr 10, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I've signed the CLA. Let me know if a NEWS entry is required. I'm not sure it is, especially since there was already a previous change that this is a fix to.

@picnixz
picnixz self-requested a review May 17, 2025 11:00
picnixz
picnixz reviewed May 17, 2025
View reviewed changes

@picnixzpicnixz left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a NEWS entry please? It's still worth to mention that we fixed more crashes.

Comment threadLib/test/test_bytes.py Outdated
Comment threadLib/test/test_bytes.py Outdated
@bast0006

bast0006 commented May 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Done

@bast0006
bast0006 requested a review from picnixzMay 17, 2025 23:25
ZeroIntensity
ZeroIntensity reviewed May 18, 2025
View reviewed changes
Comment threadMisc/NEWS.d/next/Core_and_Builtins/2025-05-17-18-22-12.gh-issue-91153.ioA_83.rst Outdated
Comment threadLib/test/test_bytes.py
@ZeroIntensityZeroIntensity added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels May 18, 2025
picnixz
picnixz reviewed Jun 2, 2025
View reviewed changes
Comment threadLib/test/test_bytes.py Outdated
Comment threadMisc/NEWS.d/next/Core_and_Builtins/2025-05-17-20-56-05.gh-issue-91153.afgtG2.rst Outdated
Comment threadObjects/bytearrayobject.c Outdated
Comment threadObjects/bytearrayobject.c Outdated
@bast0006@picnixz
pythonGH-91153: Fixup comments and NEWS entry
18e6597
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
ZeroIntensity
ZeroIntensity approved these changes Jun 3, 2025
View reviewed changes
Comment threadLib/test/test_bytes.py Outdated
self._testlimitedcapi.sequence_setitem(b, 0, Boom())

def test_mutating_index_inbounds(self):
# See gh-91153

@ZeroIntensityZeroIntensityJun 3, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a brief description of the issue?

@bast0006bast0006Jun 3, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment threadObjects/bytearrayobject.c
picnixz
picnixz approved these changes Jul 12, 2025
View reviewed changes

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 NITS and I'll merge.

Comment threadObjects/bytearrayobject.c Outdated
Comment threadObjects/bytearrayobject.c Outdated
@picnixz
picnixz enabled auto-merge (squash) July 12, 2025 13:15
@picnixz
picnixz merged commit 5e1e21d into python:mainJul 12, 2025
41 checks passed
@miss-islington-app

miss-islington-appBot commented Jul 12, 2025

Copy link
Copy Markdown

Thanks @bast0006 for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 12, 2025
@bast0006@picnixz@miss-islington
pythongh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
02aa323
…when `ind.__index__` has side-effects (pythonGH-132379)
(cherry picked from commit 5e1e21d)
Co-authored-by: Bast <52266665+bast0006@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@miss-islington-app

miss-islington-appBot commented Jul 12, 2025

Copy link
Copy Markdown

Sorry, @bast0006 and @picnixz, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 5e1e21dee35b8e9066692d08033bbbdb562e2c28 3.13

@bedevere-app

bedevere-appBot commented Jul 12, 2025

Copy link
Copy Markdown

GH-136581 is a backport of this pull request to the 3.14 branch.

@bedevere-appbedevere-appBot removed the needs backport to 3.14 bugs and security fixes label Jul 12, 2025
picnixz added a commit to picnixz/cpython that referenced this pull request Jul 12, 2025
@bast0006@picnixz
[3.13]pythongh-91153: prevent a crash in `bytearray.__setitem__(ind,…
0e827aa
… ...)` when `ind.__index__` has side-effects (pythonGH-132379)
(cherry picked from commit 5e1e21d)
Co-authored-by: Bast <52266665+bast0006@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app

bedevere-appBot commented Jul 12, 2025

Copy link
Copy Markdown

GH-136582 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Jul 12, 2025
picnixz added a commit that referenced this pull request Jul 12, 2025
@picnixz@bast0006
[3.13]gh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
360540f
… when `ind.__index__` has side-effects (GH-132379) (#136582)
(cherry picked from commit 5e1e21d)
Co-authored-by: Bast <52266665+bast0006@users.noreply.github.com>
picnixz added a commit that referenced this pull request Jul 12, 2025
@miss-islington@bast0006@picnixz
[3.14]gh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
d5866a8
… when `ind.__index__` has side-effects (GH-132379) (#136581)
gh-91153: prevent a crash in `bytearray.__setitem__(ind, ...)` when `ind.__index__` has side-effects (GH-132379)
(cherry picked from commit 5e1e21d)
Co-authored-by: Bast <52266665+bast0006@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@bast0006@picnixz@Pranjal095
pythongh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
fc46948
…when `ind.__index__` has side-effects (python#132379)
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz added a commit to picnixz/cpython that referenced this pull request Jul 13, 2025
@bast0006@picnixz
pythongh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
2aa177d
…when `ind.__index__` has side-effects (python#132379)
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@bast0006@picnixz@taegyunkim
pythongh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
a70a3ef
…when `ind.__index__` has side-effects (python#132379)
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@bast0006@picnixz@Agent-Hellboy
pythongh-91153: prevent a crash inbytearray.__setitem__(ind, ...)…
cfc56bb
…when `ind.__index__` has side-effects (python#132379)
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
kumaraditya303 pushed a commit to miss-islington/cpython that referenced this pull request Sep 9, 2025
@miss-islington@bast0006
@picnixz@kumaraditya303
[3.14]pythongh-91153: prevent a crash in `bytearray.__setitem__(ind,…
4278eb4
… ...)` when `ind.__index__` has side-effects (pythonGH-132379) (python#136581)
pythongh-91153: prevent a crash in `bytearray.__setitem__(ind, ...)` when `ind.__index__` has side-effects (pythonGH-132379)
(cherry picked from commit 5e1e21d)
Co-authored-by: Bast <52266665+bast0006@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixzpicnixzpicnixz approved these changes
@ZeroIntensityZeroIntensityZeroIntensity approved these changes

Assignees

@picnixzpicnixz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bast0006@picnixz@ZeroIntensity