Mercurial > octave
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");