[libc++] Improve the tests for std::basic_stringbuf's constructors and assignment operators

[pfusik]

This fixes use-after-moves spotted by @Mordante in https://reviews.llvm.org/D151896#inline-1482124
The pointers are now set to null, same as in the default constructor of stringbuf.

[philnik]

This change requires a test.

[pfusik]

Test source pointers set to null.

[philnik]

I am quite certain that basic_stringbuf has to be in a valid but unspecified state, meaning that you can't just check that the pointer are null. You have to find a way to exploit the issue without directly testing the values. For example, would the moved-to buffer get touched when writing to the moved-from stringbuf (or some other operation)?

[philnik]

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/move.pass.cpp

110–114

Thanks for improving the coverage here BTW.

[pfusik]

@philnik Are the tests for libc++ or the C++ standard? The former does specify the state.

Before this change, the pointers were pointing to the string *after* the move, i.e. an empty string. I don't understand what would you like to test here?

[pfusik]

In D154499#4474816, @philnik wrote:

I am quite certain that basic_stringbuf has to be in a valid but unspecified state

https://timsong-cpp.github.io/cppwp/n4868/stringbuf#cons-11.13

[Mordante]

In D154499#4474853, @pfusik wrote:

@philnik Are the tests for libc++ or the C++ standard? The former does specify the state.

Before this change, the pointers were pointing to the string *after* the move, i.e. an empty string. I don't understand what would you like to test here?

Thanks for working on this!

tests in the directory test/std are for the C++ standard. You can use LIBCPP_ASSERT for libc++ specific asserts.
tests in the directory test/libcxx are always libc++ specific tests.

[pfusik]

s/assert/LIBCPP_ASSERT/

[philnik]

In D154499#4474861, @pfusik wrote:

In D154499#4474816, @philnik wrote:

I am quite certain that basic_stringbuf has to be in a valid but unspecified state

https://timsong-cpp.github.io/cppwp/n4868/stringbuf#cons-11.13

Interesting. IIUC the buffer has to be empty after a move. We can test for that, but it looks to me like that's already the case with the old code. Since you claim that this is a bug fix, this has to be observable in some way by the user without checking for unspecified behavior. Otherwise this is basically an NFC without any reasoning for why the new code is better. It actually looks to me like the old code is correct.

[pfusik]

Mark move assignment/constructor tests as unsupported in C++03.

[pfusik]

@Mordante You pointed this as a problem in https://reviews.llvm.org/D151896#inline-1482124 Could you please explain? Now I'm confused myself.

BTW. Was stringbuf copy-assignable in C++03 ? See https://en.cppreference.com/w/cpp/io/basic_stringbuf/operator%3D

[ldionne]

Commandeering to finish for the GH PR transition.

After consideration, I think our current tests are lacking a bit but there is no risk of use-after-move. Indeed, when we set __p = const_cast<char_type*>(__rhs.__str_.data());, __p is:

• nullptr if the string was not using SSO, because it was moved-out-from above in __str_ = _VSTD::move(__rhs.__str_);
• a pointer to the start of the SSO inside __rhs.__str_ if the string was using SSO

Either way, we are not reusing the storage incorrectly. I will commandeer and fix the tests, but I think our current implementation is fine.

[ldionne]

Updating.

[ldionne]

Fix C++11 CI issues.

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/move.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
+// UNSUPPORTED: c++03
+
 // <sstream>
 
 // template <class charT, class traits = char_traits<charT>, class Allocator = allocator<charT> >
 // class basic_stringbuf
 
 // basic_stringbuf& operator=(basic_stringbuf&& rhs);
 
 #include <sstream>
 #include <cassert>
+#include <utility>
 
+#include "make_string.h"
 #include "test_macros.h"
 
-int main(int, char**)
-{
-    {
-        std::stringbuf buf1("testing");
-        std::stringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == "testing");
-    }
+#define STR(S) MAKE_STRING(CharT, S)
+
+template <class CharT>
+struct test_stringbuf : std::basic_stringbuf<CharT> {
+  using std::basic_stringbuf<CharT>::basic_stringbuf;
+
+  // Checks the following requirement after being moved from:
+  //    The six pointers of std::basic_streambuf in *this are guaranteed to be different
+  //    from the corresponding pointers in the moved-from rhs unless null.
+  void check_different_pointers(test_stringbuf<CharT> const& other) const {
+    assert(this->eback() == nullptr || this->eback() != other.eback());
+    assert(this->gptr() == nullptr || this->gptr() != other.gptr());
+    assert(this->egptr() == nullptr || this->egptr() != other.egptr());
+    assert(this->pbase() == nullptr || this->pbase() != other.pbase());
+    assert(this->pptr() == nullptr || this->pptr() != other.pptr());
+    assert(this->epptr() == nullptr || this->epptr() != other.epptr());
+};
+
+template <class CharT>
+void test() {
+  std::basic_string<CharT> strings[] = {STR(""), STR("short"), STR("loooooooooooooooooooong")};
+  for (std::basic_string<CharT> const& s : strings) {
     {
-        std::stringbuf buf1("testing", std::ios_base::in);
-        std::stringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == "testing");
+      test_stringbuf<CharT> buf1(s);
+      test_stringbuf<CharT> buf;
+      assert(buf.str() == s);
+      buf.check_different_pointers(buf1);
     }
     {
-        std::stringbuf buf1("testing", std::ios_base::out);
-        std::stringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == "testing");
+      test_stringbuf<CharT> buf1(s, std::ios_base::in);
+      test_stringbuf<CharT> buf;
+      assert(buf.str() == s);
+      buf.check_different_pointers(buf1);
     }
-#ifndef TEST_HAS_NO_WIDE_CHARACTERS
     {
-        std::wstringbuf buf1(L"testing");
-        std::wstringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == L"testing");
+      test_stringbuf<CharT> buf1(s, std::ios_base::out);
+      test_stringbuf<CharT> buf;
+      assert(buf.str() == s);
+      buf.check_different_pointers(buf1);
     }
     {
-        std::wstringbuf buf1(L"testing", std::ios_base::in);
-        std::wstringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == L"testing");
+      test_stringbuf<CharT> buf1;
+      test_stringbuf<CharT> buf;
+      buf.check_different_pointers(buf1);
     }
+    // Use the assignment operator on an actual std::stringbuf, not test_stringbuf
     {
-        std::wstringbuf buf1(L"testing", std::ios_base::out);
-        std::wstringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == L"testing");
-    }
-#endif // TEST_HAS_NO_WIDE_CHARACTERS
+      std::basic_stringbuf<CharT> buf1(s);
+      std::basic_stringbuf<CharT> buf;
+      assert(buf.str() == s);
+    }
+  }
 
+int main(int, char**) {
+  test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  test<wchar_t>();
+#endif
   return 0;
 }

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/move.alloc.pass.cpp

 
 // template <class charT, class traits = char_traits<charT>, class Allocator = allocator<charT>>
 // class basic_stringbuf
 
 // basic_stringbuf(basic_stringbuf&& rhs, const Allocator& a);
 
 #include <sstream>
 #include <cassert>
+#include <utility>
 
 #include "make_string.h"
 #include "test_allocator.h"
 #include "test_macros.h"
 
 #define STR(S) MAKE_STRING(CharT, S)
 #define SV(S) MAKE_STRING_VIEW(CharT, S)
 
 template <class CharT>
-static void test() {
-  std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>> buf1(STR("testing"));
+struct test_stringbuf : std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>> {
+  using std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>>::basic_stringbuf;
+
+  // Checks the following requirement after being moved from:
+  //    The six pointers of std::basic_streambuf in *this are guaranteed to be different
+  //    from the corresponding pointers in the moved-from rhs unless null.
+  void check_different_pointers(test_stringbuf<CharT> const& other) const {
+    assert(this->eback() == nullptr || this->eback() != other.eback());
+    assert(this->gptr() == nullptr || this->gptr() != other.gptr());
+    assert(this->egptr() == nullptr || this->egptr() != other.egptr());
+    assert(this->pbase() == nullptr || this->pbase() != other.pbase());
+    assert(this->pptr() == nullptr || this->pptr() != other.pptr());
+    assert(this->epptr() == nullptr || this->epptr() != other.epptr());
+  }
+};
+
+template <class CharT>
+void test() {
+  std::basic_string<CharT> strings[] = {STR(""), STR("short"), STR("loooooooooooooooooooong")};
+  for (std::basic_string<CharT> const& s : strings) {
+    using StringBuf = std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>>;
+    {
+      test_stringbuf<CharT> buf1(s);
+      const test_allocator<CharT> a(2);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)), a);
+      assert(buf.get_allocator() == a);
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
+    }
+    {
+      test_stringbuf<CharT> buf1(s, std::ios_base::in);
+      const test_allocator<CharT> a(2);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)), a);
+      assert(buf.get_allocator() == a);
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
+    }
+    {
+      test_stringbuf<CharT> buf1(s, std::ios_base::out);
       const test_allocator<CharT> a(2);
-  const std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>> buf(std::move(buf1), a);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)), a);
       assert(buf.get_allocator() == a);
-  assert(buf.view() == SV("testing"));
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
+    }
+    {
+      test_stringbuf<CharT> buf1;
+      const test_allocator<CharT> a(2);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)), a);
+      assert(buf.get_allocator() == a);
+      assert(buf.view().empty());
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
+    }
+    // Use the constructor from an actual std::stringbuf, not test_stringbuf
+    {
+      StringBuf buf1(s);
+      const test_allocator<CharT> a(2);
+      StringBuf buf(std::move(buf1), a);
+      assert(buf.get_allocator() == a);
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+    }
+  }
 }
 
 int main(int, char**) {
   test<char>();
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test<wchar_t>();
 #endif
   return 0;
 }

libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/move.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
+// UNSUPPORTED: c++03
+
 // <sstream>
 
 // template <class charT, class traits = char_traits<charT>, class Allocator = allocator<charT> >
 // class basic_stringbuf
 
 // basic_stringbuf(basic_stringbuf&& rhs);
 
 #include <sstream>
 #include <cassert>
+#include <utility>
 
+#include "make_string.h"
 #include "test_macros.h"
 
-int main(int, char**)
-{
-    {
-        std::stringbuf buf1("testing");
-        std::stringbuf buf(std::move(buf1));
-        assert(buf.str() == "testing");
-    }
+#define STR(S) MAKE_STRING(CharT, S)
+
+template <class CharT>
+struct test_stringbuf : std::basic_stringbuf<CharT> {
+  using std::basic_stringbuf<CharT>::basic_stringbuf;
+
+  test_stringbuf(std::basic_stringbuf<CharT>&& other) : std::basic_stringbuf<CharT>(std::move(other)) {}
+
+  // Checks the following requirement after being moved from:
+  //    The six pointers of std::basic_streambuf in *this are guaranteed to be different
+  //    from the corresponding pointers in the moved-from rhs unless null.
+  void check_different_pointers(test_stringbuf<CharT> const& other) const {
+    assert(this->eback() == nullptr || this->eback() != other.eback());
+    assert(this->gptr() == nullptr || this->gptr() != other.gptr());
+    assert(this->egptr() == nullptr || this->egptr() != other.egptr());
+    assert(this->pbase() == nullptr || this->pbase() != other.pbase());
+    assert(this->pptr() == nullptr || this->pptr() != other.pptr());
+    assert(this->epptr() == nullptr || this->epptr() != other.epptr());
+};
+
+template <class CharT>
+void test() {
+  std::basic_string<CharT> strings[] = {STR(""), STR("short"), STR("loooooooooooooooooooong")};
+  for (std::basic_string<CharT> const& s : strings) {
+    using StringBuf = std::basic_stringbuf<CharT>;
     {
-        std::stringbuf buf1("testing", std::ios_base::in);
-        std::stringbuf buf(std::move(buf1));
-        assert(buf.str() == "testing");
+      test_stringbuf<CharT> buf1(s);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)));
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
     }
     {
-        std::stringbuf buf1("testing", std::ios_base::out);
-        std::stringbuf buf(std::move(buf1));
-        assert(buf.str() == "testing");
+      test_stringbuf<CharT> buf1(s, std::ios_base::in);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)));
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
     }
-#ifndef TEST_HAS_NO_WIDE_CHARACTERS
     {
-        std::wstringbuf buf1(L"testing");
-        std::wstringbuf buf(std::move(buf1));
-        assert(buf.str() == L"testing");
+      test_stringbuf<CharT> buf1(s, std::ios_base::out);
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)));
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
     }
     {
-        std::wstringbuf buf1(L"testing", std::ios_base::in);
-        std::wstringbuf buf(std::move(buf1));
-        assert(buf.str() == L"testing");
+      test_stringbuf<CharT> buf1;
+      test_stringbuf<CharT> buf(std::move(static_cast<StringBuf&>(buf1)));
+      assert(buf.view().empty());
+      assert(buf1.view().empty());
+      buf.check_different_pointers(buf1);
     }
+    // Use the constructor from an actual std::stringbuf, not test_stringbuf
     {
-        std::wstringbuf buf1(L"testing", std::ios_base::out);
-        std::wstringbuf buf(std::move(buf1));
-        assert(buf.str() == L"testing");
-    }
-#endif
+      StringBuf buf1(s);
+      StringBuf buf(std::move(buf1));
+      assert(buf.view() == s);
+      assert(buf1.view().empty());
+    }
+  }
 
+int main(int, char**) {
+  test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  test<wchar_t>();
+#endif
   return 0;
 }

libcxx/utils/data/ignore_format.txt

 libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.assign/move.pass.cpp
 libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.assign/nonmember_swap.pass.cpp
 libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.cons/default.pass.cpp
 libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.cons/move.pass.cpp
 libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.cons/string.pass.cpp
 libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.pass.cpp
 libcxx/test/std/input.output/string.streams/ostringstream/types.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/member_swap.pass.cpp
-libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/move.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.assign/nonmember_swap.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/default.pass.cpp
-libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/move.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.cons/string.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.virtuals/overflow.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.virtuals/pbackfail.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.virtuals/seekoff.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.virtuals/seekpos.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.virtuals/setbuf.pass.cpp
 libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.virtuals/underflow.pass.cpp