[libcxx-commits] [libcxx] e097619 - [libc++] Improves UB handling in ios_base destructor. (#76525)

[via libcxx-commits]

Author: Mark de Wever
Date: 2024-03-12T18:04:14+01:00
New Revision: e09761944cea4aeafadd055b9510ef9f0e9a7338

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

LOG: [libc++] Improves UB handling in ios_base destructor. (#76525)

Destroying an ios_base object before it is properly initialized is
undefined behavior. Unlike typical C++ classes the initialization is not
done in the constructor, but in a dedicated init function. Due to
virtual inheritance of the basic_ios object in ostream and friends this
undefined behaviour can be triggered when inheriting from classes that
can throw in their constructor and inheriting from ostream.

Use the __loc_ member of ios_base as sentinel to detect whether the
object has or has not been initialized.

Addresses https://github.com/llvm/llvm-project/issues/57964

Added: 
    libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp

Modified: 
    libcxx/include/ios
    libcxx/src/ios.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/include/ios b/libcxx/include/ios
index 4b1306fc2ad830..00c1d5c2d4bc58 100644
--- a/libcxx/include/ios
+++ b/libcxx/include/ios
@@ -359,7 +359,13 @@ public:
   }
 
 protected:
-  _LIBCPP_HIDE_FROM_ABI ios_base() { // purposefully does no initialization
+  _LIBCPP_HIDE_FROM_ABI ios_base() : __loc_(nullptr) {
+    // Purposefully does no initialization
+    //
+    // Except for the locale, this is a sentinel to avoid destroying
+    // an uninitialized object. See
+    // test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp
+    // for the details.
   }
 
   void init(void* __sb);
@@ -571,7 +577,9 @@ public:
   _LIBCPP_HIDE_FROM_ABI char_type widen(char __c) const;
 
 protected:
-  _LIBCPP_HIDE_FROM_ABI basic_ios() { // purposefully does no initialization
+  _LIBCPP_HIDE_FROM_ABI basic_ios() {
+    // purposefully does no initialization
+    // since the destructor does nothing this does not have ios_base issues.
   }
   _LIBCPP_HIDE_FROM_ABI void init(basic_streambuf<char_type, traits_type>* __sb);
 

diff  --git a/libcxx/src/ios.cpp b/libcxx/src/ios.cpp
index d58827fa1255cf..a727855c4655e8 100644
--- a/libcxx/src/ios.cpp
+++ b/libcxx/src/ios.cpp
@@ -195,6 +195,10 @@ void ios_base::register_callback(event_callback fn, int index) {
 }
 
 ios_base::~ios_base() {
+  // Avoid UB when not properly initialized. See ios_base::ios_base for
+  // more information.
+  if (!__loc_)
+    return;
   __call_callbacks(erase_event);
   locale& loc_storage = *reinterpret_cast<locale*>(&__loc_);
   loc_storage.~locale();

diff  --git a/libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp b/libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp
new file mode 100644
index 00000000000000..ea42203d054469
--- /dev/null
+++ b/libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp
@@ -0,0 +1,80 @@
+//===----------------------------------------------------------------------===//
+//
+// 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: no-exceptions
+
+// The fix for issue 57964 requires an updated dylib due to explicit
+// instantiations. That means Apple backdeployment targets remain broken.
+// UNSUPPORTED: using-built-library-before-llvm-19
+
+// <ios>
+
+// class ios_base
+
+// ~ios_base()
+//
+// Destroying a constructed ios_base object that has not been
+// initialized by basic_ios::init is undefined behaviour. This can
+// happen in practice, make sure the undefined behaviour is handled
+// gracefully.
+//
+//
+// [ios.base.cons]/1
+//
+// ios_base();
+// Effects: Each ios_base member has an indeterminate value after construction.
+// The object's members shall be initialized by calling basic_ios::init before
+// the object's first use or before it is destroyed, whichever comes first;
+// otherwise the behavior is undefined.
+//
+// [basic.ios.cons]/2
+//
+// basic_ios();
+// Effects: Leaves its member objects uninitialized.  The object shall be
+// initialized by calling basic_ios::init before its first use or before it is
+// destroyed, whichever comes first; otherwise the behavior is undefined.
+//
+// ostream and friends have a basic_ios virtual base.
+// [class.base.init]/13
+// In a non-delegating constructor, initialization proceeds in the
+// following order:
+// - First, and only for the constructor of the most derived class
+//   ([intro.object]), virtual base classes are initialized ...
+//
+// So in this example
+// struct Foo : AlwaysThrows, std::ostream {
+//    Foo() : AlwaysThrows{}, std::ostream{nullptr} {}
+// };
+//
+// Here
+// - the ios_base object is constructed
+// - the AlwaysThrows object is constructed and throws an exception
+// - the AlwaysThrows object is destrodyed
+// - the ios_base object is destroyed
+//
+// The ios_base object is destroyed before it has been initialized and runs
+// into undefined behavior. By using __loc_ as a sentinel we can avoid
+// accessing uninitialized memory in the destructor.
+
+#include <ostream>
+
+struct AlwaysThrows {
+  AlwaysThrows() { throw 1; }
+};
+
+struct Foo : AlwaysThrows, std::ostream {
+  Foo() : AlwaysThrows(), std::ostream(nullptr) {}
+};
+
+int main(int, char**) {
+  try {
+    Foo foo;
+  } catch (...) {
+  };
+  return 0;
+}