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

[Mark de Wever via libcxx-commits]

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

>From f4b031015e7dc713a979ac3ed1a6f2bac6095ce9 Mon Sep 17 00:00:00 2001
From: Mark de Wever <koraq at xs4all.nl>
Date: Thu, 28 Dec 2023 19:14:33 +0100
Subject: [PATCH] [libc++] Improves UB handling in ios_base destructor.

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
---
 libcxx/include/ios                            | 49 ++++++++++++++++++-
 libcxx/src/ios.cpp                            |  4 ++
 .../ios.base.cons/dtor.uninitialized.pass.cpp | 41 ++++++++++++++++
 3 files changed, 92 insertions(+), 2 deletions(-)
 create mode 100644 libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp

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..eef8f5bb20cb57
--- /dev/null
+++ b/libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp
@@ -0,0 +1,41 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// The fix for issue 57964 requires an updated dylib due to explicit
+// instantiations. That means Apple backdeployment targets remain broken.
+// UNSUPPORTED: stdlib=apple-libc++ && target={{.+}}-apple-macosx10.{{.+}}
+// UNSUPPORTED: stdlib=apple-libc++ && target={{.+}}-apple-macosx11.{{.+}}
+
+// <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;
+}