Re: [PATCH v3] libstdc++: Fix SIGSEGV in std::print with a setvbuf'd FILE
Tomasz Kaminski <[email protected]>
| Newsgroups | gmane.comp.gcc.patches,gmane.comp.gcc.libstdc++.devel |
|---|---|
| Message-ID | <CAKvuMXDZBFA7U5eg02fKLXnZepe=pU0Agf4pB0v84teHk+zz6w@mail.gmail.com> |
On Thu, Aug 20, 2026 at 5:28 PM Jonathan Wakely <[email protected]> wrote: > On Thu, 20 Aug 2026 at 15:21, Tomasz Kaminski <[email protected]> wrote: > > > > > > > > On Wed, Aug 19, 2026 at 4:40 PM Jonathan Wakely <[email protected]> > wrote: > >> > >> On Wed, 19 Aug 2026 at 15:24, Tomasz Kaminski <[email protected]> > wrote: > >> > > >> > > >> > > >> > On Wed, Aug 19, 2026 at 4:17 PM Jonathan Wakely <[email protected]> > wrote: > >> >> > >> >> On Mon, 17 Aug 2026 at 09:58 +0200, Tomasz Kamiński wrote: > >> >> >From: Anlai Lu <[email protected]> > >> >> > > >> >> >_File_sink::_M_write_buf() returns a span with a null data pointer > when > >> >> >the FILE buffer was pre-allocated by setvbuf but never written > (glibc > >> >> >keeps _IO_write_ptr as nullptr until the first write). The empty() > check > >> >> >in the constructor then fails to force buffer initialization and > the format > >> >> >engine memcpys into nullptr. > >> >> > >> >> Please replace the paragraph above with the first two paragraphs > >> >> from the original [PATCH 0/1] email, and a shorter form of the third > >> >> paragraph: > >> >> > >> >> This is a regression introduced by the P3107R5 implementation in > >> >> r16-4350-g8bd872f1ea7414. Before that formatting to a FILE* wrote > to > >> >> _Str_sink and used fwrite, which handles this legal stream state. > >> >> > >> >> > >> >> >Calls __overflow when _IO_write_ptr is null in _File_sink > constructor. > >> >> > >> >> Let's say: > >> >> > >> >> This fixes it by calling __overflow from the _File_sink > constructor > >> >> when _IO_write_ptr is null. > >> >> > >> >> > > >> >> >libstdc++-v3/ChangeLog: > >> >> > > >> >> > * include/bits/print.h (_File_sink::_File::_File) > >> >> > [_GLIBCXX_USE_STDIO_LOCKING && _GLIBCXX_USE_GLIBC_STDIO_EXT]: > >> >> > Move __overflow call to separte member function. > >> >> > (_File_sink::_File::_M_init_write_buf) > >> >> > [_GLIBCXX_USE_STDIO_LOCKING && _GLIBCXX_USE_GLIBC_STDIO_EXT]: > >> >> > Extracted from constructor, call __overflow for null > >> >> > _M_file->_IO_write_ptr. > >> >> > (_File_sink::_File_sink) > >> >> > [_GLIBCXX_USE_STDIO_LOCKING && _GLIBCXX_USE_GLIBC_STDIO_EXT]: > >> >> > Call _M_init_write_buf instead of _M_writebuf. > >> >> > * testsuite/27_io/print/1.cc: Add test_print_setvbuf. > >> >> > > >> >> >Co-authored-by: Tomasz Kamiński <[email protected]> > >> >> >Signed-off-by: Anlai Lu <[email protected]> > >> >> >Signed-off-by: Tomasz Kamiński <[email protected]> > >> >> >--- > >> >> >v3 introduced _M_init_write_buf that I suggested on review. > >> >> > > >> >> >Tested on x86_64-linux. OK for trunk and 16? > >> >> > > >> >> > libstdc++-v3/include/bits/print.h | 28 > ++++++++++++++++--------- > >> >> > libstdc++-v3/testsuite/27_io/print/1.cc | 17 +++++++++++++++ > >> >> > 2 files changed, 35 insertions(+), 10 deletions(-) > >> >> > > >> >> >diff --git a/libstdc++-v3/include/bits/print.h > b/libstdc++-v3/include/bits/print.h > >> >> >index 20724844726..d63267c8ecf 100644 > >> >> >--- a/libstdc++-v3/include/bits/print.h > >> >> >+++ b/libstdc++-v3/include/bits/print.h > >> >> >@@ -80,20 +80,28 @@ namespace __format > >> >> > ::funlockfile(__f); > >> >> > __throw_system_error(EACCES); > >> >> > } > >> >> >- // Allocate buffer if needed: > >> >> >- if (_M_write_buf().empty()) > >> >> >- if (::__overflow(__f, EOF) == EOF) > >> >> >- { > >> >> >- const int __err = errno; > >> >> >- ::funlockfile(__f); > >> >> >- __throw_system_error(__err); > >> >> >- } > >> >> > } > >> >> > > >> >> > ~_File() { ::funlockfile(_M_file); } > >> >> > > >> >> > _File(_File&&) = delete; > >> >> > > >> >> >+ // Allocate FILE's output buffer if needed, and returns span > >> >> > >> >> s/returns span/return a span/ > >> >> > >> >> >+ // viewing unused portion of it. > >> >> >+ std::span<char> > >> >> >+ _M_init_write_buf() > >> >> >+ { > >> >> >+ // After setvbuf glibc pre-allocates the buffer but > _IO_write_ptr > >> >> >+ // remains null until the first write. > >> >> > >> >> The two comment lines above are indented inconsistently, the first > one > >> >> uses spaces and the second uses a tab. > >> >> > >> >> >+ if (!_M_file->_IO_write_ptr || _M_write_buf().empty()) > >> >> >+ if (::__overflow(_M_file, EOF) == EOF) > >> >> >+ { > >> >> >+ const int __err = errno; > >> >> >+ __throw_system_error(__err); > >> >> > >> >> There's no need to store errno before throwing (because we're not > >> >> calling funlockfile here) so it can be just: > >> >> > >> >> __throw_system_error(errno); > >> >> > >> >> OK with those changes, thanks. > >> > > >> > OK also to backport after some time to 16? > >> > >> Yes, OK for gcc-16 too. > >> No need to wait, I think this is obviously more correct and safer than > >> the current code. > > > > I was thinking about test, i.e. that there is some architecture on which > > setvbuf does not work, and I will need to add some dg-requires to it. > > I'd still just push to gcc-16 now. If it causes a new FAIL, we can fix > it on both branches. > Backported it (with follow up squashed) to GCC-16 just now. > > setvbuf is in C89 and the testcase has dg-require-fileio, which > requires POSIX open, lseek and read. I would expect any system > providing those POSIX APIs to also provide C89 stdio. > > >> > >> > >> >> > >> >> > >> >> > >> >> >+ } > >> >> >+ return _M_write_buf(); > >> >> >+ } > >> >> >+ > >> >> > // A span viewing the unused portion of the stream's output > buffer. > >> >> > std::span<char> > >> >> > _M_write_buf() noexcept > >> >> >@@ -158,8 +166,8 @@ namespace __format > >> >> > : _M_file(__f), _M_add_newline(__add_newline) > >> >> > { > >> >> > if (!_M_file._M_unbuffered()) > >> >> >- // Write directly to the FILE's output buffer. > >> >> >- this->_M_reset(_M_file._M_write_buf()); > >> >> >+ // Allocate FILE's output buffer if needed, and write > directly to it. > >> >> >+ this->_M_reset(_M_file._M_init_write_buf()); > >> >> > } > >> >> > > >> >> > // This calls I/O functions which are cancellation points, so > they > >> >> >diff --git a/libstdc++-v3/testsuite/27_io/print/1.cc > b/libstdc++-v3/testsuite/27_io/print/1.cc > >> >> >index 4fd7f5dc925..641a97c1f9c 100644 > >> >> >--- a/libstdc++-v3/testsuite/27_io/print/1.cc > >> >> >+++ b/libstdc++-v3/testsuite/27_io/print/1.cc > >> >> >@@ -67,6 +67,22 @@ test_print_raw() > >> >> > VERIFY( txt == "\xa3" ); > >> >> > } > >> >> > > >> >> >+void > >> >> >+test_print_setvbuf() > >> >> >+{ > >> >> >+ __gnu_test::scoped_file f; > >> >> >+ FILE* strm = std::fopen(f.path.string().c_str(), "w"); > >> >> >+ VERIFY( strm ); > >> >> >+ VERIFY( std::setvbuf(strm, nullptr, _IOFBF, 4096) == 0 ); > >> >> >+ std::string str{"Hello, World!"}; > >> >> >+ std::print(strm, "{}", str); > >> >> >+ std::fclose(strm); > >> >> >+ > >> >> >+ std::ifstream in(f.path); > >> >> >+ std::string txt(std::istreambuf_iterator<char>(in), {}); > >> >> >+ VERIFY( txt == "Hello, World!" ); > >> >> >+} > >> >> >+ > >> >> > void > >> >> > test_vprint_nonunicode() > >> >> > { > >> >> >@@ -142,6 +158,7 @@ int main() > >> >> > test_print_file(); > >> >> > test_println_file(); > >> >> > test_print_raw(); > >> >> >+ test_print_setvbuf(); > >> >> > test_vprint_nonunicode(); > >> >> > #ifdef __cpp_exceptions > >> >> > test_errors(); > >> >> >-- > >> >> >2.55.0 > >> >> > > >> >> > > >> >> > >> > >