libc++ 15.0.1 segfaults in `std::__1::ios_base::~ios_base()` when object was not yet properly constructed yet

[marv]

When building a project what works perfectly fine with GCC/libstdc++ with Clang/libc++ I'm getting a segfault in std::__1::ios_base::~ios_base(). I've boiled the code that causes the crash down to this reproducer:

#include <cerrno>
#include <cstring>
#include <fcntl.h>
#include <filesystem>
#include <iostream>
#include <istream>
#include <sys/stat.h>
#include <sys/types.h>
#include <system_error>
#include <unistd.h>

namespace fs = std::filesystem;

class SafeIFStreamBuf : public std::streambuf {
public:
  SafeIFStreamBuf(const int fd);

  int fd;

protected:
  static const int lookbehind_size = 16;
  static const int buffer_size = 512 + lookbehind_size;
  char buffer[buffer_size];
};

class SafeIFStreamBase {
protected:
  SafeIFStreamBuf buf;

public:
  SafeIFStreamBase(const int fd);
};

class SafeIFStream : protected SafeIFStreamBase, public std::istream {
private:
  const bool _close;

public:
  explicit SafeIFStream(const int fd);
  explicit SafeIFStream(const fs::path &);
  ~SafeIFStream() override;
};

SafeIFStreamBuf::SafeIFStreamBuf(const int f) : fd(f) {
  int n_read =
      read(fd, buffer + lookbehind_size, buffer_size - lookbehind_size);
  if (-1 == n_read)
    throw std::system_error(errno, std::generic_category(),
                            "Error reading from fd " + std::to_string(fd));
}

SafeIFStreamBase::SafeIFStreamBase(const int f) : buf(f) {}

SafeIFStream::SafeIFStream(const int f)
    : SafeIFStreamBase(f), std::istream(&buf), _close(false) {}

namespace {
int open_path(const fs::path &e) {
  int result = open(e.string().c_str(), O_RDONLY | O_CLOEXEC);
  if (-1 == result)
    throw std::system_error(errno, std::generic_category(),
                            "Could not open '" + e.string() + "'");

  return result;
}
} // namespace

SafeIFStream::SafeIFStream(const fs::path &e)
    : SafeIFStreamBase(open_path(e)), std::istream(&buf), _close(true) {}

SafeIFStream::~SafeIFStream() {
  if (_close)
    ::close(buf.fd);
}

int main() {
  fs::path test_dir = fs::current_path() / "test_directory";
  fs::create_directory(test_dir);

  try {
    SafeIFStream ifs(test_dir);
    std::string t;
    ifs >> t;
  } catch (const std::system_error &ex) {
    std::cout << "expected exception thrown" << std::endl;
    return 0;
  }

  std::cout << "expected exception not thrown" << std::endl;
  return 1;
}

Building with Clang/libc++ 15.0.1 vs. GCC/libstdc++ 12.2.0:

$ clang++-15 -std=c++17 -stdlib=libc++ -o reproducer reproducer.cc 
$ ./reproducer 
Segmentation fault (core dumped)
$ g++-12 -std=c++17 -o reproducer_gcc reproducer.cc 
$ ./reproducer_gcc 
expected exception thrown

The important part is the inheritance from std::istream and the exception that's thrown during the construction of the SafeIFStreamBuf class. In this situation ios_base's member variables are not properly initialized and ~ios_base tries to emit its callbacks. But since __event_size_ contains garbage it segfaults trying to access __fn_[i]

The following patch fixes the crash:

diff --git a/libcxx/include/ios b/libcxx/include/ios
index 7140e00b406a..b1a796418ba3 100644
--- a/libcxx/include/ios
+++ b/libcxx/include/ios
@@ -350,8 +350,11 @@ public:
 
 protected:
     _LIBCPP_INLINE_VISIBILITY
-    ios_base() {// purposefully does no initialization
-               }
+    ios_base() :
+        __event_size_{0}
+    {
+        // purposefully does no initialization
+    }
 
     void init(void* __sb);
     _LIBCPP_INLINE_VISIBILITY void* rdbuf() const {return __rdbuf_;}
@@ -381,12 +384,12 @@ private:
     streamsize      __width_;
     iostate         __rdstate_;
     iostate         __exceptions_;
-    void*           __rdbuf_;
-    void*           __loc_;
-    event_callback* __fn_;
-    int*            __index_;
-    size_t          __event_size_;
-    size_t          __event_cap_;
+    void*           __rdbuf_ = nullptr;
+    void*           __loc_ = nullptr;
+    event_callback* __fn_ = nullptr;
+    int*            __index_ = nullptr;
+    size_t          __event_size_ = 0;
+    size_t          __event_cap_ = 0;
 // TODO(EricWF): Enable this for both Clang and GCC. Currently it is only
 // enabled with clang.
 #if defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_NO_THREADS)
@@ -394,12 +397,12 @@ private:
 #else
     static int      __xindex_;
 #endif
-    long*           __iarray_;
-    size_t          __iarray_size_;
-    size_t          __iarray_cap_;
-    void**          __parray_;
-    size_t          __parray_size_;
-    size_t          __parray_cap_;
+    long*           __iarray_ = nullptr;
+    size_t          __iarray_size_ = 0;
+    size_t          __iarray_cap_ = 0;
+    void**          __parray_ = nullptr;
+    size_t          __parray_size_ = 0;
+    size_t          __parray_cap_ = 0;
 };
 
 //enum class io_errc
diff --git a/libcxx/src/ios.cpp b/libcxx/src/ios.cpp
index 218b27f1a6b5..40ddbe53052d 100644
--- a/libcxx/src/ios.cpp
+++ b/libcxx/src/ios.cpp
@@ -233,8 +233,10 @@ ios_base::register_callback(event_callback fn, int index)
 ios_base::~ios_base()
 {
     __call_callbacks(erase_event);
-    locale& loc_storage = *reinterpret_cast<locale*>(&__loc_);
-    loc_storage.~locale();
+    if (__loc_ != nullptr) {
+        locale& loc_storage = *reinterpret_cast<locale*>(&__loc_);
+        loc_storage.~locale();
+    }
     free(__fn_);
     free(__index_);
     free(__iarray_);

The first hunk is a left-over, because I started with an initializer list but noticed that more variables than __event_size_ needed to be initialized and switched to default-member-initialization. This is more of an FYI/RFC instead of a real attempt to fix it, because I'm not familiar with the code at all.

In libstdc++'s code the ios_base constructor mentions the need for proper initialization of the member variables:

  ios_base::ios_base() throw()
  : _M_precision(), _M_width(), _M_flags(), _M_exception(),
  _M_streambuf_state(), _M_callbacks(0), _M_word_zero(),
  _M_word_size(_S_local_word_size), _M_word(_M_local_word), _M_ios_locale()
  {
    // Do nothing: basic_ios::init() does it.
    // NB: _M_callbacks and _M_word must be zero for non-initialized
    // ios_base to go through ~ios_base gracefully.
  }

Best regards,
Marvin

[piotrrak]

Hi,

This one is still present also x86_64

$ clang-18 --version
Ubuntu clang version 18.0.0 (++20230810042320+9be8892908d4-1~exp1~20230810042442.1113)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Here's much smaller example:

#include <ostream>

struct AlwaysThrows { AlwaysThrows() {throw 1;} };

struct Foo : AlwaysThrows, std::ostream
{
   Foo() : AlwaysThrows{}, std::ostream{nullptr} {}
};

int main() { try {Foo foo{}; }catch(...){}; return 0; }

The suggested fix above seems to be correct.

[mordante]

I had a look at it. It seems not initializing the members is on purpose and destroying the object before it's properly initialized in UB :-( Still I think like libstdc++ we can do better.