[libcxx-commits] [libcxx] 4ff70db - [libc++] Fix undefined behavior in `std::filebuf`

[Fabian Wolff via libcxx-commits]

Author: Fabian Wolff
Date: 2022-04-14T16:20:51+02:00
New Revision: 4ff70dba3839ce9510499a79f93522b67cab504c

URL: https://github.com/llvm/llvm-project/commit/4ff70dba3839ce9510499a79f93522b67cab504c
DIFF: https://github.com/llvm/llvm-project/commit/4ff70dba3839ce9510499a79f93522b67cab504c.diff

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

Fixes https://github.com/llvm/llvm-project/issues/49267.
Fixes https://github.com/llvm/llvm-project/issues/49282.
Fixes https://github.com/llvm/llvm-project/issues/49789.

Reviewed By: ldionne

Differential Revision: https://reviews.llvm.org/D122257

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

Modified: 
    libcxx/include/fstream
    libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap.pass.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/include/fstream b/libcxx/include/fstream
index 907c59e888fb0..7608daa3f97f8 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -419,25 +419,38 @@ 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
     {
-        ptr
diff _t __ln = __extbufnext_ - __extbuf_;
-        ptr
diff _t __le = __extbufend_ - __extbuf_;
-        ptr
diff _t __rn = __rhs.__extbufnext_ - __rhs.__extbuf_;
-        ptr
diff _t __re = __rhs.__extbufend_ - __rhs.__extbuf_;
+        ptr
diff _t __ln = __extbufnext_       ? __extbufnext_ - __extbuf_             : 0;
+        ptr
diff _t __le = __extbufend_        ? __extbufend_ - __extbuf_              : 0;
+        ptr
diff _t __rn = __rhs.__extbufnext_ ? __rhs.__extbufnext_ - __rhs.__extbuf_ : 0;
+        ptr
diff _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;

diff  --git a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap.pass.cpp
index 4ae3fc22f0664..003b0302c8acd 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap.pass.cpp
@@ -39,6 +39,50 @@ int main(int, char**)
     }
     std::remove(temp.c_str());
 
+    // 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');
+    }
+    std::remove(temp.c_str());
+
+    // neither lhs nor rhs use the small buffer
+    {
+        std::string tmpA = get_temp_file_name();
+        std::string tmpB = get_temp_file_name();
+
+        {
+            // currently the small buffer has size 8
+            std::ofstream sa(tmpA), sb(tmpB);
+            sa << "0123456789";
+            sb << "abcdefghij";
+        }
+
+        std::filebuf f1, f2;
+        assert(f1.open(tmpA, std::ios_base::in) != 0);
+        assert(f2.open(tmpB, std::ios_base::in) != 0);
+        assert(f1.sgetc() == '0');
+        assert(f2.sgetc() == 'a');
+
+        swap(f1, f2);
+
+        assert(f1.sgetc() == 'a');
+        assert(f2.sgetc() == '0');
+
+        std::remove(tmpA.c_str());
+        std::remove(tmpB.c_str());
+    }
+
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
     {
         std::wfilebuf f;

diff  --git a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp
new file mode 100644
index 0000000000000..6981360cef4ff
--- /dev/null
+++ b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.assign/nonmember_swap_min.pass.cpp
@@ -0,0 +1,62 @@
+//===----------------------------------------------------------------------===//
+//
+// 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).
+
+// <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 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(tmpA.c_str());
+    std::remove(tmpB.c_str());
+
+    return 0;
+}