[libc++] Fix ungetc failing after xsgetn (#210951)

After #206453 we don't correctly handle `unget()` anymore. This fixes
the issue by updating the internal buffer to contain the tail of the
read data.

Fixes #210203

libcxx/include/fstream

@@ -311,6 +311,8 @@ protected:
 
   _LIBCPP_HIDE_FROM_ABI_VIRTUAL streamsize xsgetn(char_type* __str, streamsize __len) override {
     if (__file_ && __always_noconv_) {
+      char_type __1buf;
+      __read_guard __guard(__1buf, *this);
       const streamsize __n = std::min(this->egptr() - this->gptr(), __len);
       if (__n != 0) {
         traits_type::copy(__str, this->gptr(), __n);
@@ -319,9 +321,16 @@ protected:
       const streamsize __remainder    = __len - __n;
       const streamsize __buffer_space = this->egptr() - this->eback();
 
-      if (__remainder >= __buffer_space)
-        return std::fread(__str + __n, sizeof(char_type), __remainder, __file_) + __n;
-      else if (__remainder > 0)
+      if (__remainder >= __buffer_space) {
+        auto __res = std::fread(__str + __n, sizeof(char_type), __remainder, __file_) + __n;
+
+        // Copy the buffer-sized tail into the buffer so that `unget`ting works correctly
+        auto __buf_size = std::min<size_t>(__res, this->egptr() - this->eback());
+        traits_type::copy(this->eback(), __str + __res - __buf_size, __buf_size);
+        this->setg(this->eback(), this->eback() + __buf_size, this->eback() + __buf_size);
+
+        return __res;
+      } else if (__remainder > 0)
         return basic_streambuf<_CharT, _Traits>::xsgetn(__str + __n, __remainder) + __n;
       return __n;
     }
@@ -384,6 +393,40 @@ private:
   bool __always_noconv_;
 
   bool __read_mode();
+
+  struct [[__nodiscard__]] __read_guard {
+    basic_filebuf& __self_;
+    size_t __unget_size_;
+    char_type& __1buf_;
+
+    _LIBCPP_HIDE_FROM_ABI __read_guard(char_type& __1buf, basic_filebuf& __self) : __self_(__self), __1buf_(__1buf) {
+      if (__self.__cm_ & ios_base::in) {
+        __unget_size_ = std::min<size_t>((__self.egptr() - __self.eback()) / 2, 4);
+      } else {
+        __self.setp(nullptr, nullptr);
+        if (__self.__always_noconv_) {
+          __self.setg(reinterpret_cast<char_type*>(__self.__extbuf_),
+                      reinterpret_cast<char_type*>(__self.__extbuf_) + __self.__ebs_,
+                      reinterpret_cast<char_type*>(__self.__extbuf_) + __self.__ebs_);
+        } else {
+          __self.setg(__self.__intbuf_, __self.__intbuf_ + __self.__ibs_, __self.__intbuf_ + __self.__ibs_);
+        }
+        __self.__cm_  = ios_base::in;
+        __unget_size_ = 0;
+      }
+
+      if (__self.gptr() == nullptr)
+        __self.setg(std::addressof(__1buf_), std::addressof(__1buf_) + 1, std::addressof(__1buf_) + 1);
+    }
+
+    _LIBCPP_HIDE_FROM_ABI size_t __unget_size() { return __unget_size_; }
+
+    _LIBCPP_HIDE_FROM_ABI ~__read_guard() {
+      if (__self_.eback() == std::addressof(__1buf_))
+        __self_.setg(nullptr, nullptr, nullptr);
+    }
+  };
+
   void __write_mode();
 
   _LIBCPP_HIDE_FROM_ABI static int __fseek(FILE* __file, pos_type __offset, int __whence);
@@ -803,11 +846,11 @@ template <class _CharT, class _Traits>
 typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>::underflow() {
   if (__file_ == nullptr)
     return traits_type::eof();
-  bool __initial = __read_mode();
+
   char_type __1buf;
-  if (this->gptr() == nullptr)
-    this->setg(std::addressof(__1buf), std::addressof(__1buf) + 1, std::addressof(__1buf) + 1);
-  const size_t __unget_sz = __initial ? 0 : std::min<size_t>((this->egptr() - this->eback()) / 2, 4);
+  __read_guard __guard(__1buf, *this);
+
+  const size_t __unget_sz = __guard.__unget_size();
   int_type __c            = traits_type::eof();
   if (this->gptr() == this->egptr()) {
     std::memmove(this->eback(), this->egptr() - __unget_sz, __unget_sz * sizeof(char_type));
@@ -850,8 +893,6 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
     }
   } else
     __c = traits_type::to_int_type(*this->gptr());
-  if (this->eback() == std::addressof(__1buf))
-    this->setg(nullptr, nullptr, nullptr);
   return __c;
 }
 
@@ -1142,20 +1183,6 @@ void basic_filebuf<_CharT, _Traits>::imbue(const locale& __loc) {
   }
 }
 
-template <class _CharT, class _Traits>
-bool basic_filebuf<_CharT, _Traits>::__read_mode() {
-  if (!(__cm_ & ios_base::in)) {
-    this->setp(nullptr, nullptr);
-    if (__always_noconv_)
-      this->setg((char_type*)__extbuf_, (char_type*)__extbuf_ + __ebs_, (char_type*)__extbuf_ + __ebs_);
-    else
-      this->setg(__intbuf_, __intbuf_ + __ibs_, __intbuf_ + __ibs_);
-    __cm_ = ios_base::in;
-    return true;
-  }
-  return false;
-}
-
 template <class _CharT, class _Traits>
 void basic_filebuf<_CharT, _Traits>::__write_mode() {
   if (!(__cm_ & ios_base::out)) {

libcxx/src/ios.instantiations.cpp

@@ -15,6 +15,26 @@
 #include <streambuf>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
+_LIBCPP_BEGIN_EXPLICIT_ABI_ANNOTATIONS
+
+#if _LIBCPP_HAS_FILESYSTEM
+// This is provided for ABI compatiblity with programs compiled against old headers.
+// TODO: Is this actually required? If so, this should be guarded with
+// `_LIBCPP_AVAILABILITY_MINIMUM_HEADER_VERSION < 24`. Otherwise this should be removed.
+template <class _CharT, class _Traits>
+bool basic_filebuf<_CharT, _Traits>::__read_mode() {
+  if (!(__cm_ & ios_base::in)) {
+    this->setp(nullptr, nullptr);
+    if (__always_noconv_)
+      this->setg((char_type*)__extbuf_, (char_type*)__extbuf_ + __ebs_, (char_type*)__extbuf_ + __ebs_);
+    else
+      this->setg(__intbuf_, __intbuf_ + __ibs_, __intbuf_ + __ibs_);
+    __cm_ = ios_base::in;
+    return true;
+  }
+  return false;
+}
+#endif
 
 // Original explicit instantiations provided in the library
 template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_ios<char>;
@@ -45,4 +65,5 @@ template class _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS basic_filebuf<char>;
 
 // Add more here if needed...
 
+_LIBCPP_END_EXPLICIT_ABI_ANNOTATIONS
 _LIBCPP_END_NAMESPACE_STD

libcxx/test/extensions/libcxx/input.output/file.streams/fstreams/filebuf.members/test.dat

@@ -0,0 +1 @@
+This is a bunch of data so the test can read some stuff and not instantly run out of data to read.

libcxx/test/extensions/libcxx/input.output/file.streams/fstreams/filebuf.members/xsgetn.buffer.pass.cpp

@@ -0,0 +1,159 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// FILE_DEPENDENCIES: test.dat
+
+// UNSUPPORTED: no-localization, no-filesystem
+
+// XFAIL: using-built-library-before-llvm-24
+
+// <fstream>
+
+// streamsize basic_filebuf::xsgetn(char_type*, streamsize);
+
+// Test that xsgetn buffers properly. Specifically, we guarantee that `unget()` can be called at least four times.
+
+#include <cassert>
+#include <fstream>
+#include <string>
+
+#include "platform_support.h"
+
+void small_file_tests() {
+  char buffer[12];
+
+  { // Check that we can unget() when reading a single character
+    std::ifstream is("test.dat");
+    assert(is.is_open());
+    char buf[1];
+    is.read(buf, 1);
+    assert(buf[0] == 'T');
+    is.unget();
+    assert(is.good());
+  }
+
+  { // Check that we can unget() when reading a single character with a user-provided buffer
+    std::ifstream is("test.dat");
+    assert(is.is_open());
+    is.rdbuf()->pubsetbuf(buffer, 12);
+    char buf[1];
+    is.read(buf, 1);
+    assert(buf[0] == 'T');
+    is.unget();
+    assert(is.good());
+  }
+
+  { // Check that unget() works as expected when the remainder is smaller than the buffer
+    std::ifstream is("test.dat");
+    assert(is.is_open());
+    is.rdbuf()->pubsetbuf(buffer, 12);
+    (void)is.rdbuf()->sgetc(); // Make sure there is data in the buffer
+    char buf[17];
+    buf[16] = '\0';
+    is.read(buf, 16);
+    assert(buf == std::string("This is a bunch "));
+    for (size_t i = 0; i != 4; ++i)
+      is.unget();
+    assert(is.good());
+    is.read(buf, 4);
+    buf[4] = '\0';
+    assert(buf == std::string("nch "));
+  }
+
+  { // Check that unget() works as expected when the remainder is larger than the buffer
+    std::ifstream is("test.dat");
+    assert(is.is_open());
+    is.rdbuf()->pubsetbuf(buffer, 12);
+    char buf[33];
+    buf[32] = '\0';
+    is.read(buf, 32);
+    assert(buf == std::string("This is a bunch of data so the t"));
+    for (size_t i = 0; i != 4; ++i)
+      is.unget();
+    assert(is.good());
+    is.read(buf, 4);
+    buf[4] = '\0';
+    assert(is.good());
+    assert(buf == std::string("he t"));
+  }
+
+  { // read an empty file
+    std::string empty_file = get_temp_file_name();
+    {
+      std::ofstream os(empty_file);
+    }
+    std::ifstream is(empty_file);
+    is.rdbuf()->pubsetbuf(nullptr, 64);
+
+    char buf[100];
+    is.read(buf, 100);
+    assert(is.eof());
+    assert(is.gcount() == 0);
+
+    is.clear();
+    is.unget();
+    assert(is.fail());
+    std::remove(empty_file.c_str());
+  }
+}
+
+static std::string make_pattern(std::size_t n) {
+  std::string s(n, '\0');
+  for (std::size_t i = 0; i != n; ++i)
+    s[i] = static_cast<char>('0' + (i % 10));
+  return s;
+}
+
+void large_file_tests() {
+  const std::string data = make_pattern(10000);
+  std::string file       = get_temp_file_name();
+  { // Prepare the file
+    std::ofstream os(file);
+    assert(os.write(data.data(), data.size()));
+  }
+
+  { // default buffer with a read larger than the buffer
+    std::ifstream is(file);
+    assert(is.is_open());
+    std::string buf;
+    buf.resize(8000);
+
+    is.read(&*buf.begin(), 8000);
+    assert(is.gcount() == 8000);
+    assert(std::string(buf.data(), 8000) == data.substr(0, 8000));
+    is.unget();
+    assert(is.good());
+    assert(is.get() == data[7999]); // the ungotten character
+    assert(is.get() == data[8000]); // reading forward continues from the right place
+  }
+
+  { // EOF before the buffer is full
+    std::ifstream is(file);
+    assert(is.is_open());
+    is.rdbuf()->pubsetbuf(nullptr, 64);
+
+    std::string buf;
+    buf.resize(data.size() + 100);
+
+    is.read(&*buf.begin(), data.size() + 100);
+    assert(is.gcount() == static_cast<std::streamsize>(data.size()));
+    assert(is.rdstate() == (std::ios::eofbit | std::ios::failbit));
+    is.clear(); // Clear the failbit due to trying to read more data than available
+    is.unget();
+    assert(is.good());
+    assert(is.get() == data.back());
+  }
+
+  std::remove(file.c_str());
+}
+
+int main(int, char**) {
+  small_file_tests();
+  large_file_tests();
+  return 0;
+}

libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.pass.cpp

@@ -25,6 +25,14 @@
 
 #include "test_macros.h"
 
+void check_unget(std::filebuf* fb, char expected) {
+  auto c = fb->sungetc();
+  if (c != EOF) {
+    assert(c == expected);
+    assert(fb->sbumpc() == expected);
+  }
+}
+
 int main(int, char**) {
   std::vector<char> stream_buffer(10);
   std::ifstream fs("xsgetn.test.dat");
@@ -40,33 +48,39 @@ int main(int, char**) {
   { // Check that a read smaller than the buffer works fine
     assert(fb->sgetn(test_buffer.data(), 5) == 5);
     assert(std::string(test_buffer.data(), 5) == "this ");
+    check_unget(fb, ' ');
   }
   { // Check that reading up to the buffer end works fine
     assert(fb->sgetn(test_buffer.data(), 5) == 5);
     assert(std::string(test_buffer.data(), 5) == "is so");
+    check_unget(fb, 'o');
   }
   { // Check that reading from an empty buffer, but more than the buffer can
     // hold works fine
     test_buffer.resize(12);
     assert(fb->sgetn(test_buffer.data(), 12) == 12);
     assert(std::string(test_buffer.data(), 12) == "me random da");
+    check_unget(fb, 'a');
   }
   { // Check that reading from a non-empty buffer, and more than the buffer can
     // hold works fine Fill the buffer up
     test_buffer.resize(2);
     assert(fb->sgetn(test_buffer.data(), 2) == 2);
     assert(std::string(test_buffer.data(), 2) == "ta");
+    check_unget(fb, 'a');
 
     // Do the actual check
     test_buffer.resize(12);
     assert(fb->sgetn(test_buffer.data(), 12) == 12);
     assert(std::string(test_buffer.data(), 12) == " to be able ");
+    check_unget(fb, ' ');
   }
   { // Check that trying to read more than the file size works fine
     test_buffer.resize(30);
     assert(fb->sgetn(test_buffer.data(), 30) == 24);
     test_buffer.resize(24);
     assert(std::string(test_buffer.data(), 24) == "to test buffer behaviour");
+    check_unget(fb, 'r');
   }
   { // Ensure that the read fails gracefully with an unopened ifstream
     // See https://llvm.org/PR168628