fix: segfault when moving scoped_ostream_redirect

[kounelisagis]

Description

The default move constructor of scoped_ostream_redirect caused a segfault when the moved-to object (or the original stream) was subsequently used.

The root cause: = default moved the internal pythonbuf (transferring its d_buffer, pywrite, pyflush and nulling the streambuf base pointers), but left the stream (e.g. std::cout) still pointing at the moved-from buffer. Any subsequent write through the stream called overflow() on the zombie pythonbuf, which dereferenced pptr() (now null) — immediate segfault.

Reproducer

py::scoped_ostream_redirect redir1(std::cout, py::module_::import("sys").attr("stdout"));
std::cout << "before" << std::flush;              // OK
py::scoped_ostream_redirect redir2(std::move(redir1));
std::cout << "after" << std::flush;               // SIGSEGV

Fix

Replace = default with an explicit move constructor that:

1. Moves the buffer to the new location
2. Re-points the stream to the new buffer (costream.rdbuf(&buffer))
3. Sets other.old = nullptr to disarm the moved-from destructor

The destructor is guarded with if (old) so the moved-from object skips restoring the stream.

Suggested changelog entry:

• Fixed segfault when moving scoped_ostream_redirect: the default move constructor left the stream pointing at a moved-from pythonbuf with null internal pointers, causing a null dereference on the next write.

[rwgk]

@kounelisagis Please see below. I'm using Cursor GPT-5.4 Extra High Fast. I could easily ask it to first add the a test for the old == nullptr regression, and then make your new test pass without breaking that one. Do you want to do that on your end, or do you prefer if I try that?

---

Initial Review Prompt

Could you please review very carefully? I'm not familiar with the py::scoped_ostream_redirect code. I want to rely on your review. Could you please flag anything you're uncertain about?

Findings

• High: include/pybind11/iostream.h now uses old == nullptr as the moved-from sentinel, but nullptr is also a valid previous rdbuf() value for a real std::ostream. That means a redirect created on something like std::ostream os(nullptr) will no longer restore the original nullptr buffer in the destructor, and the stream is left pointing at a destroyed pythonbuf.

    explicit scoped_ostream_redirect(std::ostream &costream = std::cout,
                                     const object &pyostream
                                     = module_::import("sys").attr("stdout"))
        : costream(costream), buffer(pyostream) {
        old = costream.rdbuf(&buffer);
    }

    ~scoped_ostream_redirect() {
        if (old) {
            costream.rdbuf(old);
        }
    }

    scoped_ostream_redirect(const scoped_ostream_redirect &) = delete;
    scoped_ostream_redirect(scoped_ostream_redirect &&other) noexcept
        : old(other.old), costream(other.costream), buffer(std::move(other.buffer)) {
        other.old = nullptr;     // Disarm moved-from destructor
        costream.rdbuf(&buffer); // Re-point stream to our buffer
    }

The old destructor restored old unconditionally, which correctly handled old == nullptr. After this patch, “moved-from” and “original buffer was null” are conflated. I sanity-checked locally that both std::ostream os(nullptr) and restoring rdbuf(nullptr) are valid on libstdc++, so this is a real regression, not just a theoretical one. I would fix this with a separate ownership flag such as active / owns_redirection, and still always restore old when that flag is set.

• Low: the new regression test only exercises the happy-path std::cout move with a normal existing buffer and a pre-flush before the move, so it would not catch the old == nullptr regression above or other state-management edge cases in this code path.

    m.def("move_redirect_output", [](const std::string &msg) {
        py::scoped_ostream_redirect redir1(std::cout, py::module_::import("sys").attr("stdout"));
        std::cout << "before" << std::flush;
        py::scoped_ostream_redirect redir2(std::move(redir1));
        std::cout << msg << std::flush;
    });

def test_move_redirect(capsys):
    m.move_redirect_output("after")
    stdout, stderr = capsys.readouterr()
    assert stdout == "beforeafter"
    assert not stderr

Open Questions

• I’m not confident nested redirects on the same stream are safe to move in general. Each guard stores a raw old pointer into another buffer, so moving a non-top-most redirect still looks fragile even if the top-of-stack move case is fixed.
• I’m also not fully sure the move path preserves pending buffered bytes on every supported STL. The test flushes "before" before the move, so it proves “next write doesn’t crash,” but not “unflushed buffered data survives a move.”
• I did not try to treat the new noexcept as a blocker, but I also did not independently prove the whole move path is non-throwing on every supported standard library implementation.

Overall

I would not merge this as-is because of the old == nullptr regression in scoped_ostream_redirect ownership/restoration logic.

Aside from that, the main idea of the fix looks right: rebinding the stream to the moved-to pythonbuf and disarming the moved-from destructor is the correct direction, and I didn’t find another concrete correctness bug in the simple top-level move case this PR is targeting.

[rwgk]

@kounelisagis please see below. This is still using Cursor GPT-5.4 Extra High Fast. What do you think about the "I would be more comfortable with something like" code? Maybe clang-tidy doesn't demand the noexcept with the suggested code? Could you please try?

---

Prompt

I'm wondering about exception safety in this code:

scoped_ostream_redirect(scoped_ostream_redirect &&other) noexcept
    : old(other.old), costream(other.costream), buffer(std::move(other.buffer)) {
    other.active = false;    // Disarm moved-from destructor
    costream.rdbuf(&buffer); // Re-point stream to our buffer
}

Does the noexcept make this universally safe?

Or do we have to worry about internal consistency between other.active and other other members, or even this members?

Response

Findings

• noexcept does not make this universally safe. It only means that if anything in the move path throws, the program terminates instead of letting the exception propagate. I checked locally on GCC 13 / libstdc++ 13, and pybind11::detail::pythonbuf is not std::is_nothrow_move_constructible.
• There is also a separate logical-state issue in the latest commit: this.active is always default-initialized to true, instead of being derived from other.active. So moving an already-moved-from redirect would incorrectly try to rebind the stream again.

    scoped_ostream_redirect(scoped_ostream_redirect &&other) noexcept
        : old(other.old), costream(other.costream), buffer(std::move(other.buffer)) {
        other.active = false;    // Disarm moved-from destructor
        costream.rdbuf(&buffer); // Re-point stream to our buffer
    }

What That Means

noexcept is only “safe” here in the narrow sense that you won’t continue running after an exception escapes this constructor. It is not a proof that the operations inside are non-throwing.

There are two separate concerns:

First, buffer(std::move(other.buffer)) is not universally non-throwing. pybind11::object itself is noexcept-movable:

    /// Move constructor; steals the object from ``other`` and preserves its reference count
    object(object &&other) noexcept : handle(other) { other.m_ptr = nullptr; }

but pythonbuf is derived from std::streambuf, and on this libstdc++ its defaulted move constructor is not treated as nothrow.

Second, costream.rdbuf(&buffer) is not specified as noexcept. On this libstdc++ implementation, basic_ios::rdbuf(newbuf) updates the stored streambuf pointer and then calls clear(). For a non-null buffer I do not see an actual throw path on libstdc++, so I’m less worried about that one in practice, but I would not call it a universal guarantee across implementations.

So my answer to your direct question is:

• No, noexcept does not make this universally safe.
• It mostly turns “possible inconsistency under exception” into “hard terminate”.

Active Flag Concern

Yes, you do still have to worry about internal consistency of active relative to the rest of the moved-from state.

Right now the move constructor assumes every source object should produce an active destination object. That is fine for the first move from a live redirect, but not for moving an already inactive object. The ownership bit should really move too.

I would be more comfortable with something like:

scoped_ostream_redirect(scoped_ostream_redirect &&other)
    : old(other.old),
      costream(other.costream),
      buffer(std::move(other.buffer)),
      active(other.active) {
    if (active) {
        costream.rdbuf(&buffer);
        other.active = false;
    }
}

Recommendation

I would remove noexcept.

If clang-tidy is the only reason for it, I would rather suppress that warning than promise a stronger exception guarantee than the actual member operations and standard-library calls provide.

I would also initialize active from other.active, not always from the default member initializer.

Uncertainty

The part I’m least certain about is whether rdbuf(&buffer) can actually throw on any standard library when &buffer is non-null. On libstdc++ it looks effectively non-throwing in this case, but I would still not use that as the basis for a blanket noexcept claim.