From 43db8f70d86b2492b79f59342187b919fd58b3dd Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 23 Oct 2025 16:34:20 +0100 Subject: [PATCH] gdbsupport: remove undefined behaviour from (forward_)scope_exit Our implementation of scope_exit and forward_scope_exit make use of the Curiously Recurring Template Pattern (CRTP) to provide the "release" functionality, this is done in the scope_exit_base class in scope-exit.h. The interesting (for this commit) parts of scope_exit_base look like this: template class scope_exit_base { public: scope_exit_base () = default; ~scope_exit_base () { if (!m_released) { auto *self = static_cast (this); self->on_exit (); } } ... snip ... }; By the time ~scope_exit_base is called the destructor for the derived class (called CRTP here) has already been run, which means any data members in the derived class will have also have had their destructors run. As a result of this I believe that casting 'this' to the derived type, and then calling the on_exit method is undefined behaviour, as 'this' can no longer be considered a valid instance of CRTP (the CRTP part has been destructed). In reality, the memory for the derived type is not reclaimed until after ~scope_exit_base has finished, so as long as the data members of the derived type are Plain Old Data (POD), then the current code should be just fine; any data members of the derived class will remain in place, and untouched, until ~scope_exit_base has completed. But still, I don't think we should rely on undefined behaviour. I actually ran into this when, in another series, I tried to reuse scope_exit_base with a class where a data member was not POD, and in this case GDB would crash because my new on_exit function was making use of the non-POD data member after it had been destructed, and resources released. I propose that we move away from the CRTP, and instead flip the class hierarchy. Instead of derived classes like scope_exit inheriting from scope_exit_base, scope_exit_base should inherit from scope_exit. What this means, is that when ~scope_exit_base is called, ~scope_exit will not yet have been run, and the data members of scope_exit will still be valid. To allow the existing names to be used, the plan is that the existing scope_exit and forward_scope_exit classes are renamed to scope_exit_policy and forward_scope_exit_policy. These policy classes will then be injected via a template argument as the base class for scope_exit_base. Finally, we can: template using scope_exit = scope_exit_base>; which defines the scope_exit type. This type is a drop in replacement with all of GDB's existing code, but avoids the undefined behaviour. We can do something similar with forward_scope_exit. There should be no user visible changes after this commit. --- gdbsupport/forward-scope-exit.h | 24 +++++----- gdbsupport/scope-exit.h | 85 ++++++++++++++------------------- 2 files changed, 49 insertions(+), 60 deletions(-) diff --git a/gdbsupport/forward-scope-exit.h b/gdbsupport/forward-scope-exit.h index 040a1616bfe..fd93374afc9 100644 --- a/gdbsupport/forward-scope-exit.h +++ b/gdbsupport/forward-scope-exit.h @@ -79,39 +79,39 @@ namespace detail Those are used to generate the constructor. */ template -struct forward_scope_exit; +struct forward_scope_exit_policy; template -class forward_scope_exit - : public scope_exit_base> +class forward_scope_exit_policy { - /* For access to on_exit(). */ - friend scope_exit_base>; - public: - explicit forward_scope_exit (Args ...args) + explicit forward_scope_exit_policy (Args ...args) : m_bind_function (function, args...) { /* Nothing. */ } -private: + DISABLE_COPY_AND_ASSIGN (forward_scope_exit_policy); + void on_exit () { m_bind_function (); } +private: /* The function and the arguments passed to the ctor, all packed in a std::bind. */ decltype (std::bind (function, std::declval ()...)) m_bind_function; }; +template +using forward_scope_exit + = scope_exit_base>; + } /* namespace detail */ /* This is the "public" entry point. It's a macro to avoid having to diff --git a/gdbsupport/scope-exit.h b/gdbsupport/scope-exit.h index 6a550c5d4c0..b61535dfadf 100644 --- a/gdbsupport/scope-exit.h +++ b/gdbsupport/scope-exit.h @@ -49,55 +49,54 @@ See also forward_scope_exit. */ -/* CRTP base class for cancelable scope_exit-like classes. Implements - the common call-custom-function-from-dtor functionality. Classes - that inherit this implement the on_exit() method, which is called - from scope_exit_base's dtor. */ +/* A template class that implements 'scope_exit' like things. The POLICY + type is used as base class, and its constructors are pulled into + scope_exit_base. -template -class scope_exit_base + When scope_exit_base is destructed, POLICY::on_exit is called so long as + release has not first been called. + + Private inheritance from POLICY as we don't want to leak any of its + details to the public API. */ + +template +class scope_exit_base : private POLICY { public: - scope_exit_base () = default; + using POLICY::POLICY; ~scope_exit_base () { if (!m_released) - { - auto *self = static_cast (this); - self->on_exit (); - } + POLICY::on_exit (); } DISABLE_COPY_AND_ASSIGN (scope_exit_base); - /* If this is called, then the wrapped function will not be called - on destruction. */ + /* If this is called, then on_exit will not be invoked from the dtor. */ void release () noexcept { m_released = true; } private: - - /* True if released. Mutable because of the copy ctor hack - above. */ - mutable bool m_released = false; + /* Only when false will on_exit be invoked from the dtor. */ + bool m_released = false; }; -/* The scope_exit class. */ +namespace detail +{ + +/* Policy class to use with scope_exit_base that will be used to implement + the scope_exit type described in the comment at the head of this file. */ template -class scope_exit : public scope_exit_base> +class scope_exit_policy { - /* For access to on_exit(). */ - friend scope_exit_base>; - public: - template>> - scope_exit (EFP &&f) + scope_exit_policy (EFP &&f) try : m_exit_function ((!std::is_lvalue_reference::value && std::is_nothrow_constructible::value) ? std::move (f) @@ -114,40 +113,22 @@ public: throw; } - template>> - scope_exit (scope_exit &&rhs) - noexcept (std::is_nothrow_move_constructible::value - || std::is_nothrow_copy_constructible::value) - : m_exit_function (std::is_nothrow_constructible::value - ? std::move (rhs) - : rhs) - { - rhs.release (); - } + DISABLE_COPY_AND_ASSIGN (scope_exit_policy); - DISABLE_COPY_AND_ASSIGN (scope_exit); - void operator= (scope_exit &&) = delete; - -private: + /* Called at scope exit unless 'release' (see scope_exit_base) was + called. */ void on_exit () { m_exit_function (); } +private: /* The function to call on scope exit. */ EF m_exit_function; }; -template -scope_exit::type> -make_scope_exit (EF &&f) -{ - return scope_exit::type> (std::forward (f)); -} - -namespace detail -{ +template +using scope_exit = scope_exit_base<::detail::scope_exit_policy>; enum class scope_exit_lhs {}; @@ -160,6 +141,14 @@ operator+ (scope_exit_lhs, EF &&rhs) } +template +::detail::scope_exit::type> +make_scope_exit (EF &&f) +{ + return ::detail::scope_exit::type> + (std::forward (f)); +} + /* Register a block of code to run on scope exit. Note that the local context is captured by reference, which means you should be careful to avoid inadvertently changing a captured local's value before the -- 2.47.3