Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

gh-111942: Fix SystemError in the TextIOWrapper constructor #112061

Merged
merged 3 commits into from
Nov 14, 2023
Merged

gh-111942: Fix SystemError in the TextIOWrapper constructor #112061

merged 3 commits into from
Nov 14, 2023

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Nov 14, 2023 •
edited by bedevere-app bot
Loading

In non-debug more the check for the "errors" argument is skipped, and then PyUnicode_AsUTF8() can fail, but its result was not checked.

@serhiy-storchaka serhiy-storchaka added type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Nov 14, 2023
@bedevere-app bedevere-app bot mentioned this pull request Nov 14, 2023
Closed
@serhiy-storchaka
pythongh-111942: Fix SystemError in the TextIOWrapper constructor
0a85bf3 
In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.
@serhiy-storchaka serhiy-storchaka changed the title gh-111942: Fix crash in the TextIOWrapper constructor gh-111942: Fix SystemError in the TextIOWrapper constructor Nov 14, 2023
@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Nov 14, 2023
vstinner
vstinner reviewed Nov 14, 2023
View reviewed changes
Modules/_io/textio.c Outdated Show resolved Hide resolved
@vstinner vstinner mentioned this pull request Nov 14, 2023
Closed
@vstinner vstinner enabled auto-merge (squash) November 14, 2023 18:45
@vstinner vstinner disabled auto-merge November 14, 2023 18:45
@vstinner vstinner enabled auto-merge (squash) November 14, 2023 18:45
@vstinner
Copy link
Member

The previous change broke the Python workflow: #111976 (comment) This change should fix it.

@vstinner vstinner merged commit 9302f05 into python:main Nov 14, 2023
@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @serhiy-storchaka and @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9302f05f9af07332c414b3c19003efd1b1763cf3 3.11

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 14, 2023
@serhiy-storchaka @vstinner @miss-islington
pythongh-111942: Fix SystemError in the TextIOWrapper constructor (py…
8561de9 
…thonGH-112061)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

(cherry picked from commit 9302f05)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-app
Copy link

bedevere-app bot commented Nov 14, 2023

GH-112085 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Nov 14, 2023
@brettcannon
Copy link
Member

I'm still seeing the test_io failure: https://buildbot.python.org/all/#/builders/1046/builds/3496/steps/11/logs/stdio .

@brettcannon
Copy link
Member

Nm, it looks like it's passing now on a different buildbot (not sure if the "changes" tab was somehow stale on the buildbot master and it misattributed the contributing PRs).

@vstinner
Copy link
Member

Nm, it looks like it's passing now on a different buildbot (not sure if the "changes" tab was somehow stale on the buildbot master and it misattributed the contributing PRs).

Time to time, I check the [Build Properties] tab to get the Git commit number. Sometimes, tracking buildbots can be confusing.

@bedevere-app
Copy link

bedevere-app bot commented Nov 14, 2023

GH-112089 is a backport of this pull request to the 3.12 branch.

vstinner pushed a commit to vstinner/cpython that referenced this pull request Nov 14, 2023
@serhiy-storchaka @vstinner
pythongh-111942: Fix SystemError in the TextIOWrapper constructor (py…
45075b8 
…thon#112061)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

Co-authored-by: Victor Stinner <vstinner@python.org>
(cherry picked from commit 9302f05)
vstinner pushed a commit to vstinner/cpython that referenced this pull request Nov 14, 2023
@serhiy-storchaka @vstinner
pythongh-111942: Fix SystemError in the TextIOWrapper constructor (py…
7f58ab2 
…thon#112061)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

Co-authored-by: Victor Stinner <vstinner@python.org>
(cherry picked from commit 9302f05)
@serhiy-storchaka serhiy-storchaka deleted the TextIOWrapper-init-crash branch November 15, 2023 08:48
@serhiy-storchaka
Copy link
Member Author

I suggest to reject embedded null characters:

I planned to do this in the following PR. This PR cannot be automatically backported with these changes. Do you mind to create backports manually?

@vstinner
Copy link
Member

I planned to do this in the following PR. This PR cannot be automatically backported with these changes. Do you mind to create backports manually?

I wrote PR #112089 for Python 3.12 using PyUnicode_AsUTF8AndSize() and strlen(). Would you mind to review it?

Then Python 3.11 can get the first change directly with the second fix (merged as a single PR).

serhiy-storchaka added a commit that referenced this pull request Nov 15, 2023
@vstinner @serhiy-storchaka
[3.12] gh-111942: Fix SystemError in the TextIOWrapper constructor (G…
1445d77 
…H-112061) (GH-112089)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

Co-authored-by: Victor Stinner <vstinner@python.org>
(cherry picked from commit 9302f05)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to miss-islington/cpython that referenced this pull request Nov 15, 2023
@vstinner @serhiy-storchaka
[3.12] pythongh-111942: Fix SystemError in the TextIOWrapper construc…
52cafc8 
…tor (pythonGH-112061) (pythonGH-112089)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

Co-authored-by: Victor Stinner <vstinner@python.org>
(cherry picked from commit 9302f05)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka serhiy-storchaka removed the needs backport to 3.11 only security fixes label Nov 15, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@serhiy-storchaka @vstinner @aisk
pythongh-111942: Fix SystemError in the TextIOWrapper constructor (py…
8d3b7eb 
…thon#112061)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

Co-authored-by: Victor Stinner <vstinner@python.org>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@serhiy-storchaka @vstinner @Glyphack
pythongh-111942: Fix SystemError in the TextIOWrapper constructor (py…
80ab1ca 
…thon#112061)

In non-debug more the check for the "errors" argument is skipped,
and then PyUnicode_AsUTF8() can fail, but its result was not checked.

Co-authored-by: Victor Stinner <vstinner@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

Assignees

@vstinner vstinner

Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@serhiy-storchaka @vstinner @brettcannon