SystemError in io.BufferedWriter

[YuanchengJiang]

Bug report

Bug description:

import io

class MockRawIO:
    def __init__(self):
        self.closed = NotImplemented
    def write(self, b):
        return len(data)
    def writable(self):
        return True

tp = io.BufferedWriter
writer = MockRawIO()
bufio = tp(writer, 8)
bufio.close()

SystemError: <method 'close' of '_io.BufferedWriter' objects> returned a result with an exception set

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

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

[inventshah]

Bisected to db68bfc. In 3.14, the behavior is emitting a catchable TypeError, but on main it crashes. I believe the problem is the sign of buffered_closed is lost in the IS_CLOSED macro.

[cmaloney]

What's the use case you have where self.closed does not exist? BufferedReader expects its raw to be a RawIOBase; RawIOBase inherits from IOBase which provides a closed attribute (https://docs.python.org/3/library/io.html#io.IOBase.closed).

I'm 👍on fixing the crash (_io__Buffered_close_impl should return NULL if there is an active exception). Given IS_CLOSED is used in some other macros (ex. CHECK_CLOSED) I'm a little worried about touching it. I think the -1 result from buffered_closed gets coerced to a bool by C rules / 1 so we end up in the if (r > 0) case. Inside that removing the -1 case and only setting res to None if https://docs.python.org/3/c-api/exceptions.html#c.PyErr_Occurred is false I think would be a fairly tactical fix.

[inventshah]

Actually, in 3.14 only the C implementation of BufferedWriter doesn't crash, the Python implementation does. My PR only restores the 3.14 behavior, so the C implementation does not crash, but the Python implementation still does.

Is it worth trying to patch the Python implementation as well? My feeling is this usage of BufferedWriter is abnormal. I imagine many library facilities may crash if given NotImplemented?

[cmaloney]

I want to make sure disambiguating crash: "The interpreter exits with an exception / error message" is distinct from "There's a segfault". In this case, the _io C code is misbehaved by what the interpreter loop wants: It returns non-NULL, I believe Py_None, when an exception is set when it should return NULL).

Generally _pyio and _io should match in visible behavior / exceptions (but implementations do differ between the two). In a case like this I'm less worried about them matching because it's not really a supported behavior. Best if they match but not worth a really complex PR or refactor to make that the case.

[inventshah]

I believe changing IS_CLOSED to preserve the sign for the error check is safe, since its only used in if-condition contexts where C's bool casting rules should apply (the PR), but would you prefer a patch like this?

diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c
index 5497c3d0d94..47420160213 100644
--- a/Modules/_io/bufferedio.c
+++ b/Modules/_io/bufferedio.c
@@ -555,10 +555,9 @@ _io__Buffered_close_impl(buffered *self)
     }
     /* 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);
+        if (!PyErr_Occurred())
+            res = Py_NewRef(Py_None);
         goto end;
     }

[cmaloney]

Yes, that's my preference. Both the CHECK_CLOSED macro and _io_BufferedWriter_write_impl call of IS_CLOSED could set another exception while the "closed exception" is set which I think isn't great (although supported / not a crash; just more exception context)

---

It might be worthwhile to do a more general fix to CHECK_CLOSED because the "IS_CLOSED" check in it could set an exception (and evaluate to 1) than the && maybe could cause it not error and return NULL itself... I haven't fully investigated that though / need to give it more thought... Also has moderate chance to break other behaviors / tests.