Skip to content

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

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
Merged
vstinner merged 3 commits into from
Nov 14, 2023
Merged

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

vstinner merged 3 commits into from
Nov 14, 2023

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchakaserhiy-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-storchakaserhiy-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 only security fixes labels Nov 14, 2023
@bedevere-appbedevere-appbot 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-storchakaserhiy-storchaka changed the title gh-111942: Fix crash in the TextIOWrapper constructorgh-111942: Fix SystemError in the TextIOWrapper constructorNov 14, 2023
@serhiy-storchakaserhiy-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
@vstinnervstinner mentioned this pull request Nov 14, 2023
Closed
@vstinnervstinner enabled auto-merge (squash) November 14, 2023 18:45
@vstinnervstinner disabled auto-merge November 14, 2023 18:45
@vstinnervstinner enabled auto-merge (squash) November 14, 2023 18:45
@vstinner
Copy link
Member

vstinner commented Nov 14, 2023

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

@vstinnervstinner merged commit 9302f05 into python:mainNov 14, 2023
@miss-islington-app
Copy link

miss-islington-appbot commented Nov 14, 2023

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

miss-islington-appbot commented Nov 14, 2023

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-appbot commented Nov 14, 2023

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

@bedevere-appbedevere-appbot removed the needs backport to 3.12 only security fixes label Nov 14, 2023
@brettcannon
Copy link
Member

brettcannon commented Nov 14, 2023

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

brettcannon commented Nov 14, 2023

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

vstinner commented Nov 14, 2023

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-appbot commented Nov 14, 2023

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

vstinner added 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 added 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-storchakaserhiy-storchaka deleted the TextIOWrapper-init-crash branch November 15, 2023 08:48
@serhiy-storchaka
Copy link
MemberAuthor

serhiy-storchaka commented Nov 15, 2023

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

vstinner commented Nov 15, 2023

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-storchakaserhiy-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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments

Assignees

@vstinnervstinner

Labels

type-bugAn 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