[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

64–68

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/include/sstream

_LIBCPP_HIDE_FROM_ABI void basic_stringbuf<_CharT, _Traits, _Allocator>::__move_init(basic_stringbuf&& __rhs) {
     if (__binp != -1)
         this->setg(__p + __binp, __p + __ninp, __p + __einp);
     if (__bout != -1)
     {
         this->setp(__p + __bout, __p + __eout);
         this->__pbump(__nout);
     }
     __hm_ = __hm == -1 ? nullptr : __p + __hm;
-    __p = const_cast<char_type*>(__rhs.__str_.data());
-    __rhs.setg(__p, __p, __p);
-    __rhs.setp(__p, __p);
-    __rhs.__hm_ = __p;
+    __rhs.setg(nullptr, nullptr, nullptr);
+    __rhs.setp(nullptr, nullptr);
+    __rhs.__hm_ = nullptr;
     this->pubimbue(__rhs.getloc());
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_stringbuf<_CharT, _Traits, _Allocator>&
 basic_stringbuf<_CharT, _Traits, _Allocator>::operator=(basic_stringbuf&& __rhs)
 {
     char_type* __p = const_cast<char_type*>(__rhs.__str_.data());
if (__bout != -1)
         this->setp(__p + __bout, __p + __eout);
         this->__pbump(__nout);
     }
     else
         this->setp(nullptr, nullptr);
 
     __hm_ = __hm == -1 ? nullptr : __p + __hm;
     __mode_ = __rhs.__mode_;
-    __p = const_cast<char_type*>(__rhs.__str_.data());
-    __rhs.setg(__p, __p, __p);
-    __rhs.setp(__p, __p);
-    __rhs.__hm_ = __p;
+    __rhs.setg(nullptr, nullptr, nullptr);
+    __rhs.setp(nullptr, nullptr);
+    __rhs.__hm_ = nullptr;
     this->pubimbue(__rhs.getloc());
     return *this;
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 void
 basic_stringbuf<_CharT, _Traits, _Allocator>::swap(basic_stringbuf& __rhs)
 #if _LIBCPP_STD_VER >= 20

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 "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");
-    }
-    {
-        std::stringbuf buf1("testing", std::ios_base::in);
-        std::stringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == "testing");
-    }
-    {
-        std::stringbuf buf1("testing", std::ios_base::out);
-        std::stringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == "testing");
-    }
-#ifndef TEST_HAS_NO_WIDE_CHARACTERS
-    {
-        std::wstringbuf buf1(L"testing");
-        std::wstringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == L"testing");
-    }
-    {
-        std::wstringbuf buf1(L"testing", std::ios_base::in);
-        std::wstringbuf buf;
-        buf = std::move(buf1);
-        assert(buf.str() == L"testing");
-    }
-    {
-        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
+#define STR(S) MAKE_STRING(CharT, S)
+
+template <class CharT>
+struct test_stringbuf : std::basic_stringbuf<CharT> {
+  using std::basic_stringbuf<CharT>::basic_stringbuf;
+  void check() {
+    LIBCPP_ASSERT(this->eback() == nullptr);
+    LIBCPP_ASSERT(this->gptr() == nullptr);
+    LIBCPP_ASSERT(this->egptr() == nullptr);
+    LIBCPP_ASSERT(this->pbase() == nullptr);
+    LIBCPP_ASSERT(this->pptr() == nullptr);
+    LIBCPP_ASSERT(this->epptr() == nullptr);
+};
+
+template <class CharT>
+static void test() {
+    test_stringbuf<CharT> buf1(STR("testing"));
+    std::basic_stringbuf<CharT> buf;
+    assert(buf.str() == STR("testing"));
+    buf1.check();
+    test_stringbuf<CharT> buf1(STR("testing"), std::ios_base::in);
+    std::basic_stringbuf<CharT> buf;
+    assert(buf.str() == STR("testing"));
+    buf1.check();
+    test_stringbuf<CharT> buf1(STR("testing"), std::ios_base::out);
+    std::basic_stringbuf<CharT> buf;
+    assert(buf.str() == STR("testing"));
+    buf1.check();
 
+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

 #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>
+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;
+  void check() {
+    LIBCPP_ASSERT(this->eback() == nullptr);
+    LIBCPP_ASSERT(this->gptr() == nullptr);
+    LIBCPP_ASSERT(this->egptr() == nullptr);
+    LIBCPP_ASSERT(this->pbase() == nullptr);
+    LIBCPP_ASSERT(this->pptr() == nullptr);
+    LIBCPP_ASSERT(this->epptr() == nullptr);
+  }
+};
+
+template <class CharT>
 static void test() {
-  std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>> buf1(STR("testing"));
+  test_stringbuf<CharT> buf1(STR("testing"));
   const test_allocator<CharT> a(2);
   const std::basic_stringbuf<CharT, std::char_traits<CharT>, test_allocator<CharT>> buf(std::move(buf1), a);
   assert(buf.get_allocator() == a);
   assert(buf.view() == SV("testing"));
+  buf1.check();
 }
 
 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 "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");
-    }
-    {
-        std::stringbuf buf1("testing", std::ios_base::in);
-        std::stringbuf buf(std::move(buf1));
-        assert(buf.str() == "testing");
-    }
-    {
-        std::stringbuf buf1("testing", std::ios_base::out);
-        std::stringbuf buf(std::move(buf1));
-        assert(buf.str() == "testing");
-    }
-#ifndef TEST_HAS_NO_WIDE_CHARACTERS
-    {
-        std::wstringbuf buf1(L"testing");
-        std::wstringbuf buf(std::move(buf1));
-        assert(buf.str() == L"testing");
-    }
-    {
-        std::wstringbuf buf1(L"testing", std::ios_base::in);
-        std::wstringbuf buf(std::move(buf1));
-        assert(buf.str() == L"testing");
-    }
-    {
-        std::wstringbuf buf1(L"testing", std::ios_base::out);
-        std::wstringbuf buf(std::move(buf1));
-        assert(buf.str() == L"testing");
-    }
-#endif
+#define STR(S) MAKE_STRING(CharT, S)
+
+template <class CharT>
+struct test_stringbuf : std::basic_stringbuf<CharT> {
+  using std::basic_stringbuf<CharT>::basic_stringbuf;
+  void check() {
+    LIBCPP_ASSERT(this->eback() == nullptr);
+    LIBCPP_ASSERT(this->gptr() == nullptr);
+    LIBCPP_ASSERT(this->egptr() == nullptr);
+    LIBCPP_ASSERT(this->pbase() == nullptr);
+    LIBCPP_ASSERT(this->pptr() == nullptr);
+    LIBCPP_ASSERT(this->epptr() == nullptr);
+};
+
+template <class CharT>
+static void test() {
+    test_stringbuf<CharT> buf1(STR("testing"));
+    std::basic_stringbuf<CharT> buf(std::move(buf1));
+    assert(buf.str() == STR("testing"));
+    buf1.check();
+    test_stringbuf<CharT> buf1(STR("testing"), std::ios_base::in);
+    std::basic_stringbuf<CharT> buf(std::move(buf1));
+    assert(buf.str() == STR("testing"));
+    buf1.check();
+    test_stringbuf<CharT> buf1(STR("testing"), std::ios_base::out);
+    std::basic_stringbuf<CharT> buf(std::move(buf1));
+    assert(buf.str() == STR("testing"));
+    buf1.check();
 
+int main(int, char**) {
+  test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  test<wchar_t>();
+#endif
   return 0;
 }