Skip to content

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors#140653

Merged
serhiy-storchaka merged 7 commits intopython:mainfrom
inventshah:fix-140650
Nov 5, 2025
Merged

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors#140653
serhiy-storchaka merged 7 commits intopython:mainfrom
inventshah:fix-140650

Conversation

@inventshah
Copy link
Copy Markdown
Contributor

@inventshah inventshah commented Oct 27, 2025 •
edited by bedevere-app bot
Loading

Restores the 3.14 behavior of emitting a TypeError. Preservation of the sign helps in the error checking here:

cpython/Modules/_io/bufferedio.c

Lines 556 to 563 in 3dab11f

/* gh-138720: Use IS_CLOSED to match flush CHECK_CLOSED. */
r = IS_CLOSED(self);
if (r < 0)
goto end;
if (r > 0) {
res = Py_NewRef(Py_None);
goto end;
}

db68bfc changed from buffered_closed to IS_CLOSED.

@bedevere-app bedevere-app bot mentioned this pull request Oct 27, 2025
Closed
@cmaloney cmaloney self-requested a review October 27, 2025 05:36
ashm-dev
ashm-dev suggested changes Oct 27, 2025
View reviewed changes
serhiy-storchaka
serhiy-storchaka reviewed Oct 27, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

This does not look right. If IS_CLOSED() returns a positive value and sets an error, there is a bug in IS_CLOSED().

@cmaloney
Copy link
Copy Markdown
Contributor

cmaloney commented Oct 27, 2025

@serhiy-storchaka : Overall problem is code assumes IS_CLOSED can't result in an exception being set but if a user-provided implementation does unusual things (ex. closed = NotImplemented) then that breaks.

Another option here is "Exception" can be turned into a True result for "IS_CLOSED" + clear the exception but that seems like hiding an implemention issue rather than helping resolve it to me.

@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Oct 27, 2025

IS_CLOSED() should return -1 on error.

cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cmaloney cmaloney left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also update CHECK_CLOSED to handle IS_CLOSED returning non-zero + setting an exception. In particular, if there's an exception CHECK_CLOSED should always return.

@cmaloney cmaloney changed the title gh-140650: fix SystemError in io.BufferedWriter.close when closed is not implemented gh-140650: fix SystemError in io.BufferedWriter.close when closed errors Oct 27, 2025
@inventshah
Copy link
Copy Markdown
Contributor Author

inventshah commented Oct 27, 2025

Should I still be using PyErr_Occurred() or based on @serhiy-storchaka's feedback, should we make IS_CLOSED() return -1 on exception and check the sign in both CHECK_CLOSED() and _io__Buffered_close_impl()?

@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Oct 27, 2025

PyErr_Occurred() is only used when we cannot distinguish the value signalling an error from a valid value, like in PyLong_AsLong(). IS_CLOSED() returns -1 on error, 0 -- false, 1 -- true. Or so it should be.

cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
efimov-mikhail
efimov-mikhail approved these changes Oct 29, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@efimov-mikhail efimov-mikhail left a comment

Choose a reason for hiding this comment

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

LGTM

serhiy-storchaka
serhiy-storchaka reviewed Oct 29, 2025
View reviewed changes
serhiy-storchaka
serhiy-storchaka approved these changes Nov 5, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@serhiy-storchaka serhiy-storchaka merged commit 1d25b75 into python:main Nov 5, 2025
50 checks passed
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@inventshah @StanFromIreland
pythongh-140650: Fix write(), flush() and close() methods of io.Buffe…
692957b 
…redWriter (pythonGH-140653)

They could raise SystemError or crash when getting the "closed" attribute
or converting it to boolean raises an exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@efimov-mikhail efimov-mikhail efimov-mikhail approved these changes

@cmaloney cmaloney cmaloney approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

+1 more reviewer

@ashm-dev ashm-dev ashm-dev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@inventshah @cmaloney @serhiy-storchaka @efimov-mikhail @ashm-dev