changeset 24071:3f036b46a130

avoid heap-use-after-free in callback execution (bug #52024) * graphics.in.h, graphics.cc (callback_property::executing): Delete. (callback_props): New class. (executing_callbacks): Rename from executing_callback and declare as callback_props object. Change all uses.
author Pantxo Diribarne <pantxo.diribarne@gmail.com>
date Thu, 21 Sep 2017 16:12:53 +0200
parents 1091931bd63c
children dbbc7e5e2294
files libinterp/corefcn/graphics.cc libinterp/corefcn/graphics.in.h
diffstat 2 files changed, 48 insertions(+), 20 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/graphics.cc	Thu Sep 21 12:45:28 2017 -0400
+++ b/libinterp/corefcn/graphics.cc	Thu Sep 21 16:12:53 2017 +0200
@@ -1766,28 +1766,59 @@
   return false;
 }
 
-// If TRUE, we are executing any callback function, or the functions it calls.
-// Used to determine handle visibility inside callback functions.
-static bool executing_callback = false;
+class callback_props
+{
+public:
+
+  callback_props (void) : m_set () { }
+
+  callback_props (const callback_props&) = delete;
+
+  callback_props& operator = (const callback_props&) = delete;
+
+  ~callback_props (void) = default;
+
+  bool empty (void) const { return m_set.empty (); }
+
+  void insert (const callback_property *ptr)
+  {
+    m_set.insert (reinterpret_cast<intptr_t> (ptr));
+  }
+
+  void erase (const callback_property *ptr)
+  {
+    m_set.erase (reinterpret_cast<intptr_t> (ptr));
+  }
+
+  bool contains (const callback_property *ptr) const
+  {
+    return m_set.find (reinterpret_cast<intptr_t> (ptr)) != m_set.end ();
+  }
+
+private:
+
+  std::set<intptr_t> m_set;
+};
+
+// Elements of this set are pointers to currently executing
+// callback_property objects.  Used to determine handle visibility
+// inside callback functions.
+
+static callback_props executing_callbacks;
 
 void
 callback_property::execute (const octave_value& data) const
 {
   octave::unwind_protect frame;
 
-  // We are executing the callback function associated with this
-  // callback property.  When set to true, we avoid recursive calls to
-  // callback routines.
-  frame.protect_var (executing);
-
   // We are executing a callback function, so allow handles that have
   // their handlevisibility property set to "callback" to be visible.
-  frame.protect_var (executing_callback);
-
-  if (! executing)
-    {
-      executing = true;
-      executing_callback = true;
+
+  frame.add_method (executing_callbacks, &callback_props::erase, this);
+
+  if (! executing_callbacks.contains (this))
+    {
+      executing_callbacks.insert (this);
 
       if (callback.is_defined () && ! callback.isempty ())
         gh_manager::execute_callback (get_parent (), callback, data);
@@ -3217,7 +3248,7 @@
 base_properties::is_handle_visible (void) const
 {
   return (handlevisibility.is ("on")
-          || (executing_callback && ! handlevisibility.is ("off")));
+          || (! executing_callbacks.empty () && ! handlevisibility.is ("off")));
 }
 
 graphics_toolkit
--- a/libinterp/corefcn/graphics.in.h	Thu Sep 21 12:45:28 2017 -0400
+++ b/libinterp/corefcn/graphics.in.h	Thu Sep 21 16:12:53 2017 +0200
@@ -1891,10 +1891,10 @@
 public:
   callback_property (const std::string& nm, const graphics_handle& h,
                      const octave_value& m)
-    : base_property (nm, h), callback (m), executing (false) { }
+    : base_property (nm, h), callback (m) { }
 
   callback_property (const callback_property& p)
-    : base_property (p), callback (p.callback), executing (false) { }
+    : base_property (p), callback (p.callback) { }
 
   octave_value get (void) const { return callback; }
 
@@ -1930,9 +1930,6 @@
 
 private:
   octave_value callback;
-
-  // If TRUE, we are executing this callback.
-  mutable bool executing;
 };
 
 // ---------------------------------------------------------------------