changeset 30770:2aff9dda08f1 stable

avoid early data free in mxArray -> octave_value data transfer (bug #61813) When handing off ownership of the data stored in an mxArray object with a single element (for example, created with mxCreateDoubleScalar) to an octave_value object, the octave_value object may mutate from an array with a single element (as the data is stored in the mxArray object) to a scalar stored in the octave_value object. When that happens, the the mxArray data is deleted and that causes trouble if the mxArray data is needed again after it is used by by Octave in the octave_value object. This mutation and deletion is not a problem when returning values from a MEX function because there is no longer any use for the mxArray data at that point. But if the MEX function creates a value and passes it to a function like mexCallMATLAB and then also uses that object after the mexCallMATLAB returns, the MEX file would be accessing data that has been deleted by the mutation process. This change fixes that problem by disabling the deletion operator in the PMR memory resource object except when values are returned from a MEX function back to Octave. * mex.cc (mx_deleting_memory_resource): Rename from mx_memory_resource. Change all uses. (mx_preserving_memory_resource): New class. (the_mx_deleting_memory_resource): Rename from the_mx_memory_resource. Change all uses. (the_mx_preserving_memory_resource): New static object. (current_mx_memory_resource): New static pointer to refer to either the_mx_deleting_memory_resource or the_mx_preserving_memory_resource. (mx_to_ov_args): New static function. (mexCallMATLAB, mexPutVariable, mexSet): Use the_mx_preserving_memory_resource while allowing octave_value objects to borrow mxArray object data. (fp_to_ov<T>, int_to_ov<T>): Don't disown data if the current allocator is the_mx_preserving_memory_resource.
author John W. Eaton <jwe@octave.org>
date Mon, 21 Feb 2022 13:01:26 -0500
parents 822649f5f193
children 9a95ccd6c417 e36380193f2b
files libinterp/corefcn/mex.cc
diffstat 1 files changed, 116 insertions(+), 16 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/mex.cc	Mon Feb 21 12:41:12 2022 -0500
+++ b/libinterp/corefcn/mex.cc	Mon Feb 21 13:01:26 2022 -0500
@@ -340,7 +340,7 @@
 
 #if defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
 
-class mx_memory_resource : public std::pmr::memory_resource
+class mx_deleting_memory_resource : public std::pmr::memory_resource
 {
 private:
 
@@ -362,13 +362,40 @@
 
   bool do_is_equal (const std::pmr::memory_resource& other) const noexcept
   {
-    return this == dynamic_cast<const mx_memory_resource *> (&other);
+    return this == dynamic_cast<const mx_deleting_memory_resource *> (&other);
+  }
+};
+
+class mx_preserving_memory_resource : public std::pmr::memory_resource
+{
+private:
+
+  void * do_allocate (std::size_t bytes, size_t /*alignment*/)
+  {
+    void *ptr = xmalloc (bytes);
+
+    if (! ptr)
+      throw std::bad_alloc ();
+
+    return ptr;
+  }
+
+  void do_deallocate (void* /*ptr*/, std::size_t /*bytes*/,
+                      std::size_t /*alignment*/)
+  { }
+
+  bool do_is_equal (const std::pmr::memory_resource& other) const noexcept
+  {
+    return this == dynamic_cast<const mx_preserving_memory_resource *> (&other);
   }
 };
 
 // FIXME: Is it OK for the memory resource object to be defined this
 // way?
-static mx_memory_resource the_mx_memory_resource;
+static mx_deleting_memory_resource the_mx_deleting_memory_resource;
+static mx_preserving_memory_resource the_mx_preserving_memory_resource;
+
+static std::pmr::memory_resource *current_mx_memory_resource = &the_mx_deleting_memory_resource;
 
 #endif
 
@@ -2144,9 +2171,14 @@
 
 #if defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
 
-    octave::unwind_action act ([=] () { maybe_disown_ptr (m_pr); });
-
-    return octave_value (Array<ELT_T> (ppr, dv, &the_mx_memory_resource));
+    if (current_mx_memory_resource == &the_mx_deleting_memory_resource)
+      {
+        octave::unwind_action act ([=] () { maybe_disown_ptr (m_pr); });
+
+        return octave_value (Array<ELT_T> (ppr, dv, current_mx_memory_resource));
+      }
+    else
+      return octave_value (Array<ELT_T> (ppr, dv, current_mx_memory_resource));
 
 #else
 
@@ -2178,14 +2210,17 @@
 
 #if 0 && defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
 
+    // FIXME: Currently not allowed because we don't have the necessary
+    // constructors for integer arrays.
+
     octave::unwind_action act ([=] () { maybe_disown_ptr (m_pr); });
 
-    return ARRAY_T (ppr, dv, &the_mx_memory_resource);
+    return ARRAY_T (ppr, dv, current_mx_memory_resource);
 
 #else
 
-    // All octave_int types are objects so we can't mix malloc and
-    // delete[] and we always have to copy.
+    // Copy data instead of allowing the octave_value object to borrow
+    // the mxArray object data.
 
     ARRAY_T val (dv);
 
@@ -4768,6 +4803,10 @@
 
   retval.resize (nargout);
 
+  // If using std::pmr::memory_resource object to manage memory, pass
+  // default allocator here because we are done with these mxArray
+  // values and want Octave to delete them.
+
   for (int i = 0; i < nargout; i++)
     retval(i) = mxArray::as_octave_value (argout[i], false);
 
@@ -4782,16 +4821,40 @@
   return mex_context ? mex_context->function_name () : "unknown";
 }
 
+static inline octave_value_list
+mx_to_ov_args (int nargin, mxArray *argin[])
+{
+  // Use a separate function for this job so that the
+  // current_mx_memory_resource will be restored immediately after the
+  // octave_value objects borrow the mxArray data.  We could also use a
+  // dummy scope in mexCallMATLAB, but this function seems less likely
+  // to be accidentally deleted.
+
+  octave_value_list args (nargin);
+
+#if defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
+
+  // Use allocator that doesn't free memory because Octave may mutate
+  // the value (single element mxArray -> scalar octave_value object,
+  // for example) and we need these objects to continue to exist after
+  // mexCallMATLAB returns.
+
+  octave::unwind_protect_var<std::pmr::memory_resource *>
+    upv (current_mx_memory_resource, &the_mx_preserving_memory_resource);
+
+#endif
+
+  for (int i = 0; i < nargin; i++)
+    args(i) = mxArray::as_octave_value (argin[i]);
+
+  return args;
+}
+
 int
 mexCallMATLAB (int nargout, mxArray *argout[], int nargin,
                mxArray *argin[], const char *fname)
 {
-  octave_value_list args;
-
-  args.resize (nargin);
-
-  for (int i = 0; i < nargin; i++)
-    args(i) = mxArray::as_octave_value (argin[i]);
+  octave_value_list args = mx_to_ov_args (nargin, argin);
 
   octave::interpreter& interp = octave::__get_interpreter__ ("mexCallMATLAB");
 
@@ -5125,7 +5188,20 @@
   octave::interpreter& interp = octave::__get_interpreter__ ("mexPutVariable");
 
   if (! strcmp (space, "global"))
-    interp.global_assign (name, mxArray::as_octave_value (ptr));
+    {
+#if defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
+
+    // Use allocator that doesn't free memory because Octave may mutate
+    // the value (single element mxArray -> scalar octave_value object,
+    // for example) and we need these objects to continue to exist after
+    // mexCallMATLAB returns.
+
+      octave::unwind_protect_var<std::pmr::memory_resource *>
+        upv (current_mx_memory_resource, &the_mx_preserving_memory_resource);
+#endif
+
+      interp.global_assign (name, mxArray::as_octave_value (ptr));
+    }
   else
     {
       // FIXME: should this be in variables.cc?
@@ -5150,6 +5226,18 @@
               tw.goto_base_frame ();
             }
 
+#if defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
+
+          // Use allocator that doesn't free memory because Octave may
+          // mutate the value (single element mxArray -> scalar
+          // octave_value object, for example) and we need these objects
+          // to continue to exist after mexCallMATLAB returns.
+
+          octave::unwind_protect_var<std::pmr::memory_resource *>
+            upv (current_mx_memory_resource,
+                 &the_mx_preserving_memory_resource);
+#endif
+
           interp.assign (name, mxArray::as_octave_value (ptr));
         }
       else
@@ -5258,6 +5346,18 @@
 int
 mexSet (double handle, const char *property, mxArray *val)
 {
+#if defined (OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR)
+
+  // Use allocator that doesn't free memory because Octave may mutate
+  // the value (single element mxArray -> scalar octave_value object,
+  // for example) and we need these objects to continue to exist after
+  // mexCallMATLAB returns.
+
+  octave::unwind_protect_var<std::pmr::memory_resource *>
+    upv (current_mx_memory_resource, &the_mx_preserving_memory_resource);
+
+#endif
+
   bool ret = octave::set_property_in_handle (handle, property,
                                              mxArray::as_octave_value (val),
                                              "mexSet");