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 <typename CRTP>
  class scope_exit_base
  {
  public:
    scope_exit_base () = default;

    ~scope_exit_base ()
    {
      if (!m_released)
        {
  	auto *self = static_cast<CRTP *> (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<typename EF>
  using scope_exit = scope_exit_base<scope_exit_policy<EF>>;

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.

diff --git a/gdbsupport/forward-scope-exit.h b/gdbsupport/forward-scope-exit.h
index 040a1616bfef31a6ac210e72060d2a7d6acf444b..fd93374afc97f5cbd7729fb5be1cedac3b7e55fe 100644 (file)
--- 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<typename Function, Function *function, typename Signature>
-struct forward_scope_exit;
+struct forward_scope_exit_policy;
 
 template<typename Function, Function *function,
         typename Res, typename... Args>
-class forward_scope_exit<Function, function, Res (Args...)>
-  : public scope_exit_base<forward_scope_exit<Function,
-                                             function,
-                                             Res (Args...)>>
+class forward_scope_exit_policy<Function, function, Res (Args...)>
 {
-  /* For access to on_exit().  */
-  friend scope_exit_base<forward_scope_exit<Function,
-                                           function,
-                                           Res (Args...)>>;
-
 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<Args> ()...))
     m_bind_function;
 };
 
+template<typename Function, Function *function,
+        typename Res, typename... Args>
+using forward_scope_exit
+       = scope_exit_base<forward_scope_exit_policy<Function,
+                                                   function, Res, Args...>>;
+
 } /* 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 6a550c5d4c0d03c84a52d5f9f93c1494c9cee85a..b61535dfadfd38b3d2ad449dd5793aefd0be28eb 100644 (file)
--- 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 <typename CRTP>
-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<typename POLICY>
+class scope_exit_base : private POLICY
 {
 public:
-  scope_exit_base () = default;
+  using POLICY::POLICY;
 
   ~scope_exit_base ()
   {
     if (!m_released)
-      {
-       auto *self = static_cast<CRTP *> (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<typename EF>
-class scope_exit : public scope_exit_base<scope_exit<EF>>
+class scope_exit_policy
 {
-  /* For access to on_exit().  */
-  friend scope_exit_base<scope_exit<EF>>;
-
 public:
-
   template<typename EFP,
           typename = gdb::Requires<std::is_constructible<EF, EFP>>>
-  scope_exit (EFP &&f)
+  scope_exit_policy (EFP &&f)
     try : m_exit_function ((!std::is_lvalue_reference<EFP>::value
                            && std::is_nothrow_constructible<EF, EFP>::value)
                           ? std::move (f)
@@ -114,40 +113,22 @@ public:
       throw;
     }
 
-  template<typename EFP,
-          typename = gdb::Requires<std::is_constructible<EF, EFP>>>
-  scope_exit (scope_exit &&rhs)
-    noexcept (std::is_nothrow_move_constructible<EF>::value
-             || std::is_nothrow_copy_constructible<EF>::value)
-    : m_exit_function (std::is_nothrow_constructible<EFP>::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 <typename EF>
-scope_exit<typename std::decay<EF>::type>
-make_scope_exit (EF &&f)
-{
-  return scope_exit<typename std::decay<EF>::type> (std::forward<EF> (f));
-}
-
-namespace detail
-{
+template<typename EF>
+using scope_exit = scope_exit_base<::detail::scope_exit_policy<EF>>;
 
 enum class scope_exit_lhs {};
 
@@ -160,6 +141,14 @@ operator+ (scope_exit_lhs, EF &&rhs)
 
 }
 
+template <typename EF>
+::detail::scope_exit<typename std::decay<EF>::type>
+make_scope_exit (EF &&f)
+{
+  return ::detail::scope_exit<typename std::decay<EF>::type>
+    (std::forward<EF> (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