[libc++] Fix undefined behavior in `std::filebuf`

[fwolff]

Summary

Attempts to fix #49267, #49282 and #49789.

[ldionne]

Thanks for looking into this! I have some comments, but overall this looks sensible. I don't know this implementation very well, apologies if some of my comments/questions are naive.

libcxx/include/fstream

429–433

Suggestion (IMO that makes it clearer because we can see a pattern in how we initialize those):

ptrdiff_t __ln = __extbufnext_       ? __extbufnext_ - __extbuf_             : 0;
ptrdiff_t __le = __extbufend_        ? __extbufend_ - __extbuf_              : 0;
ptrdiff_t __rn = __rhs.__extbufnext_ ? __rhs.__extbufnext_ - __rhs.__extbuf_ : 0;
ptrdiff_t __re = __rhs.__extbufend_  ? __rhs.__extbufend_ - __rhs.__extbuf_  : 0;

433

I know nothing about this class' implementation and I haven't studied it deeply, but this is basically a small buffer optimization, right?

I'd add comments like this:

// *this uses the small buffer, __rhs doesn't
if (...) {

}
// *this doesn't use the small buffer, __rhs does
else if (...) {

}
// *this and __rhs both use the small buffer
else {

}

Actually, this makes me think -- aren't we missing the case where neither is using the small buffer?

450

We can use std:: for new code, no need to use _VSTD:: anymore!

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp

2

This test seems to have failed on Windows: https://buildkite.com/llvm-project/libcxx-ci/builds/9676#30ce63ab-bc21-4a52-b236-4148667ef48c

12

Can you add a comment explaining what this tests, along with links to the fixed issues?

[fwolff]

Thanks for reviewing this @ldionne!

libcxx/include/fstream

433

I know nothing about this class' implementation and I haven't studied it deeply, but this is basically a small buffer optimization, right?

Right. I've added comments as you suggested.

Actually, this makes me think -- aren't we missing the case where neither is using the small buffer?

Good question, but this case is already handled by the outer if. I've added a comment there as well.

450

OK, thanks for the info. I was just "doing as the Romans do"...

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp

2

Hm, I don't have a Windows machine where I could debug this, so I unfortunately can't fix this myself. But note that the assertion before the swap fails, so my changes can't have broken it, I think? I've disabled the test on Windows for now.

[ldionne]

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp

1

Could we also add this in libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap.pass.cpp:

// lhs uses a small buffer, rhs is empty
{
    std::filebuf f;
    assert(f.open(temp.c_str(), std::ios_base::out | std::ios_base::in
                                            | std::ios_base::trunc) != 0);
    assert(f.is_open());
    assert(f.sputn("123", 3) == 3);
    f.pubseekoff(1, std::ios_base::beg);
    assert(f.sgetc() == '2');
    std::filebuf f2;
    swap(f, f2);
    assert(!f.is_open());
    assert(f2.is_open());
    assert(f2.sgetc() == '2');
}

This basically reverses the order of arguments to swap in the test that is already there. Perhaps we should also add a test that checks for swapping two filebufs that are not using the small buffer?

32

I don't think you ever use tmp, I think you could remove it altogether.

[ldionne]

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp

12–14

Also, you should make this XFAIL: LIBCXX-WINDOWS-FIXME if you can't figure it out or reproduce, and @mstorsjo might be able to help.

[mstorsjo]

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp

12–14

Don't make it LIBCXX-WINDOWS-FIXME, we don't want to add more of those. They were meant to only be used for the initial batch to get the testsuite working in such configs. We should be able to get rid of them and finally close https://github.com/llvm/llvm-project/issues/32077 real soon now, I'm just waiting for a CI runner upgrade)

A plain XFAIL with such a target triple (or just XFAIL: windows works too), with a FIXME comment, should be fine. But I can see if I can have a look at it...

[mstorsjo]

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp

12–14

See D122612 for a fix for this issue.

[ldionne]

D122612 is good to go, I think you should be able to rebase onto main soon, apply my remaining comments and then we can ship this!

[fwolff]

In D122257#3442901, @ldionne wrote:

D122612 is good to go, I think you should be able to rebase onto main soon, apply my remaining comments and then we can ship this!

Great, thanks! I think I've addressed all comments.

[ldionne]

Thanks!

libcxx/include/fstream

 
 template <class _CharT, class _Traits>
 void
 basic_filebuf<_CharT, _Traits>::swap(basic_filebuf& __rhs)
 {
     basic_streambuf<char_type, traits_type>::swap(__rhs);
     if (__extbuf_ != __extbuf_min_ && __rhs.__extbuf_ != __rhs.__extbuf_min_)
     {
-        _VSTD::swap(__extbuf_, __rhs.__extbuf_);
-        _VSTD::swap(__extbufnext_, __rhs.__extbufnext_);
-        _VSTD::swap(__extbufend_, __rhs.__extbufend_);
+        // Neither *this nor __rhs uses the small buffer, so we can simply swap the pointers.
+        std::swap(__extbuf_, __rhs.__extbuf_);
+        std::swap(__extbufnext_, __rhs.__extbufnext_);
+        std::swap(__extbufend_, __rhs.__extbufend_);
     }
     else
     {
-        ptrdiff_t __ln = __extbufnext_ - __extbuf_;
-        ptrdiff_t __le = __extbufend_ - __extbuf_;
-        ptrdiff_t __rn = __rhs.__extbufnext_ - __rhs.__extbuf_;
-        ptrdiff_t __re = __rhs.__extbufend_ - __rhs.__extbuf_;
+        ptrdiff_t __ln = __extbufnext_       ? __extbufnext_ - __extbuf_             : 0;
+        ptrdiff_t __le = __extbufend_        ? __extbufend_ - __extbuf_              : 0;
+        ptrdiff_t __rn = __rhs.__extbufnext_ ? __rhs.__extbufnext_ - __rhs.__extbuf_ : 0;
+        ptrdiff_t __re = __rhs.__extbufend_  ? __rhs.__extbufend_ - __rhs.__extbuf_  : 0;
         if (__extbuf_ == __extbuf_min_ && __rhs.__extbuf_ != __rhs.__extbuf_min_)
         {
+            // *this uses the small buffer, but __rhs doesn't.
             __extbuf_ = __rhs.__extbuf_;
             __rhs.__extbuf_ = __rhs.__extbuf_min_;
+            std::memmove(__rhs.__extbuf_min_, __extbuf_min_, sizeof(__extbuf_min_));
         }
         else if (__extbuf_ != __extbuf_min_ && __rhs.__extbuf_ == __rhs.__extbuf_min_)
         {
+            // *this doesn't use the small buffer, but __rhs does.
             __rhs.__extbuf_ = __extbuf_;
             __extbuf_ = __extbuf_min_;
+            std::memmove(__extbuf_min_, __rhs.__extbuf_min_, sizeof(__extbuf_min_));
+        }
+        else
+        {
+            // Both *this and __rhs use the small buffer.
+            char __tmp[sizeof(__extbuf_min_)];
+            std::memmove(__tmp, __extbuf_min_, sizeof(__extbuf_min_));
+            std::memmove(__extbuf_min_, __rhs.__extbuf_min_, sizeof(__extbuf_min_));
+            std::memmove(__rhs.__extbuf_min_, __tmp, sizeof(__extbuf_min_));
         }
         __extbufnext_ = __extbuf_ + __rn;
         __extbufend_ = __extbuf_ + __re;
         __rhs.__extbufnext_ = __rhs.__extbuf_ + __ln;
         __rhs.__extbufend_ = __rhs.__extbuf_ + __le;
     }
     _VSTD::swap(__ebs_, __rhs.__ebs_);
     _VSTD::swap(__intbuf_, __rhs.__intbuf_);

libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.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
+//
+//===----------------------------------------------------------------------===//
+
+// This tests that swapping filebufs works correctly even when the small buffer
+// optimization is in use (https://github.com/llvm/llvm-project/issues/49282).
+
+// FIXME: This test fails on Windows for some reason, but even before the swap,
+//        which is what we want to test here.
+// UNSUPPORTED: target={{.+}}-windows-{{.+}}
+
+// <fstream>
+
+// template <class charT, class traits = char_traits<charT> >
+// class basic_filebuf
+
+// template <class charT, class traits>
+// void
+// swap(basic_filebuf<charT, traits>& x, basic_filebuf<charT, traits>& y);
+
+#include <fstream>
+#include <cassert>
+#include "test_macros.h"
+#include "platform_support.h"
+
+int main(int, char**)
+{
+    std::string tmp = get_temp_file_name();
+    std::string tmpA = get_temp_file_name();
+    std::string tmpB = get_temp_file_name();
+
+    {
+        std::ofstream sa(tmpA), sb(tmpB);
+        sa << "AAAA";
+        sb << "BBBB";
+    }
+
+    std::filebuf f1;
+    assert(f1.open(tmpA, std::ios_base::in) != 0);
+    assert(f1.is_open());
+    f1.pubsetbuf(0, 0);
+
+    std::filebuf f2;
+    assert(f2.open(tmpB, std::ios_base::in) != 0);
+    assert(f2.is_open());
+    f2.pubsetbuf(0, 0);
+
+    assert(f1.sgetc() == 'A');
+    assert(f2.sgetc() == 'B');
+
+    swap(f1, f2);
+
+    assert(f1.is_open());
+    assert(f2.is_open());
+
+    assert(f1.sgetc() == 'B');
+    assert(f2.sgetc() == 'A');
+
+    std::remove(tmp.c_str());
+    std::remove(tmpA.c_str());
+    std::remove(tmpB.c_str());
+
+    return 0;
+}