[libcxx-commits] [libcxx] [libc++] Improves UB handling in ios_base destructor. (PR #76525)
Mark de Wever via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Mar 9 09:23:58 PST 2024
https://github.com/mordante updated https://github.com/llvm/llvm-project/pull/76525
>From c20b2832f9319af3f45dcb69a98a9d1ae2c5fd0c 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 1/2] [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 | 42 ++++++++++++++++
3 files changed, 93 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 8465860d08dc14..f6705b793a3573 100644
--- a/libcxx/include/ios
+++ b/libcxx/include/ios
@@ -360,7 +360,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);
@@ -572,7 +615,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..ad81bb40295a3b
--- /dev/null
+++ b/libcxx/test/libcxx/input.output/iostreams.base/ios.base/ios.base.cons/dtor.uninitialized.pass.cpp
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// 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. 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;
+}
>From f3f9b75d7993c4e01b4b3994ce4d04f88a04de89 Mon Sep 17 00:00:00 2001
From: Mark de Wever <zar-rpg at xs4all.nl>
Date: Sat, 9 Mar 2024 18:23:52 +0100
Subject: [PATCH 2/2] Apply suggestions from code review
Co-authored-by: Louis Dionne <ldionne.2 at gmail.com>
---
libcxx/include/ios | 2 +-
.../ios.base/ios.base.cons/dtor.uninitialized.pass.cpp | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/libcxx/include/ios b/libcxx/include/ios
index f6705b793a3573..4cbc032c51871f 100644
--- a/libcxx/include/ios
+++ b/libcxx/include/ios
@@ -361,7 +361,7 @@ public:
protected:
_LIBCPP_HIDE_FROM_ABI ios_base() : __loc_(nullptr) {
- // purposefully does no initialization
+ // Purposefully does no initialization
//
// Except for the locale, this is a sentinel to avoid destroying
// an uninitialized object.
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
index ad81bb40295a3b..dc761ecfaaa629 100644
--- 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
@@ -33,7 +33,7 @@ struct Foo : AlwaysThrows, std::ostream {
Foo() : AlwaysThrows(), std::ostream(nullptr) {}
};
-int main() {
+int main(int, char**) {
try {
Foo foo;
} catch (...) {
More information about the libcxx-commits
mailing list