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

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

<details>
<summary>Changes</summary>

Destroying and ios_base object before it's properly initialized is undefined behavior. Unlike typical C++ classes the initialization is not done in the constructor, but in an 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

---
Full diff: https://github.com/llvm/llvm-project/pull/76525.diff


3 Files Affected:

- (modified) libcxx/include/ios (+47-2) 
- (modified) libcxx/src/ios.cpp (+4) 
- (added) libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp (+36) 


``````````diff
diff --git a/libcxx/include/ios b/libcxx/include/ios
index d36f5fb2ca2842..f52ad2a2d253c2 100644
--- a/libcxx/include/ios
+++ b/libcxx/include/ios
@@ -357,7 +357,50 @@ 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.
+    //
+    // [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.
   }
 
   void init(void* __sb);
@@ -569,7 +612,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..fa6d5b63d14f35
--- /dev/null
+++ b/libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp
@@ -0,0 +1,36 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <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. See ios_base::ios_base() for the details.
+
+#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;
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/76525