bugGNU Octave - Bugs: bug #61813, memory management bug when calling...

 
 

bug #61813: memory management bug when calling MEX that returns an output

Submitter:  Sébastien Villemot <svillemot>
Submitted:  Thu 13 Jan 2022 03:31:35 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 7.0.90 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 02 Mar 2022 06:58:35 PM UTC, comment #21: 

Thanks.  Closing as fixed.

John W. Eaton <jwe>
Group administrator
Wed 02 Mar 2022 05:06:31 PM UTC, comment #20: 

I confirm that my tests now also pass on the stable branch with --enable-std-pmr-polymorphic-allocator passed at build time.

Sébastien Villemot <svillemot>
Wed 02 Mar 2022 04:23:50 PM UTC, comment #19: 

Thanks for checking.  Without std::pmr::polymorphic_allocator, the mxArray -> octave_value conversion copies data, same as in previous versions.  If it is not too much trouble, I would appreciate verification that it also works when building with --enable-std-pmr-polymorphic-allocator.

John W. Eaton <jwe>
Group administrator
Tue 01 Mar 2022 02:16:33 PM UTC, comment #18: 

Thanks, the latest changesets on the stable branch also avoids the crash for me.

OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR is not defined on my Octave builds (I don’t pass --enable-std-pmr-polymorphic-allocator to the configure script).


Sébastien Villemot <svillemot>
Mon 28 Feb 2022 07:09:09 PM UTC, comment #17: 

I pushed a set of changes to allow Sparse<T> to accept allocators and then to use a custom allocator/memory resource when transferring data from sparse mxArray objects to octave_value objects, same as we do for Array<T> now.

http://hg.savannah.gnu.org/hgweb/octave/rev/b1301358b040
http://hg.savannah.gnu.org/hgweb/octave/rev/4fc0b7269803

The custom allocator is only enabled if std::pmr::polymorphic_allocator is available and OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR is defined.  Could you update and verify that this change also avoids the crash?

Also, can you let me know whether OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR is defined in your builds?

John W. Eaton <jwe>
Group administrator
Wed 23 Feb 2022 05:22:59 PM UTC, comment #16: 

Thanks, your patch fixes the segfaults.

The sparse matrices that I manipulate are real ones. The number of rows and/or columns is typically a few hundreds.

Don’t hesitate to ask for more information if needed.


Sébastien Villemot <svillemot>
Wed 23 Feb 2022 04:46:03 PM UTC, comment #15: 

It would be helpful to have a test case but it might be enough just to know something about the sparse array you are passing back to Octave through the mexCallMATLAB function.  What are the dimensions and is it complex, real, or logical?

In any case, I realize now that I haven't extended the allocator idea to the Sparse<T> object, so either we'll have to do that or revert to always copying for sparse arrays.  Currently, we are unconditionally attempting to transfer ownership of sparse array data from mxArray objects to octave_value objects.  I'm not sure that mutation is a problem here, but mixing malloc and delete[] could cause trouble for complex data.

I'll look at using std::pmr::polymorphic_allocator for the release, but for now, can you try the attached patch and see whether it avoids the problem?  It should revert to copying always for sparse arrays.

(file #52914)

John W. Eaton <jwe>
Group administrator
Wed 23 Feb 2022 03:27:55 PM UTC, comment #14: 

Actually, I still have a problem that may be another variant of  this bug (though I’m not 100% sure).

More precisely, I get segfaults related to the manipulation of sparse matrices within calls to mexCallMATLAB().

Does that ring a bell? Or should I endeavour to create a minimal reproduction case?

Sébastien Villemot <svillemot>
Tue 22 Feb 2022 02:16:25 PM UTC, comment #13: 

Thanks for checking.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Tue 22 Feb 2022 01:58:56 PM UTC, comment #12: 

Thanks, I confirm that the bug is fixed on the stable branch (as of 2aff9dda08f1).

Sébastien Villemot <svillemot>
Mon 21 Feb 2022 06:34:50 PM UTC, comment #11: 

Thanks for checking.  I pushed a set of three changes on stable and merged with default.  They are not identical but should be equivalent to the diff I posted.  Marking as ready for test.

John W. Eaton <jwe>
Group administrator
Mon 21 Feb 2022 03:48:51 PM UTC, comment #10: 

Thanks John, confirm that your patch fixes the issue for me.

Sébastien Villemot <svillemot>
Mon 21 Feb 2022 03:49:44 AM UTC, comment #9: 

The attached change fixes the problem for me.  It reverts to always copy unless we are using the polymorphic allocator and avoids allowing Octave to delete data transferred from an mxArray to an octave_value object unless it is done when returning from a MEX function call.

The problem was that in handing off ownership of the data in an mxArray object with a single element to an octave_value object, the octave_value object may mutate from an array to a scalar object and then the mxArray data would be deleted.  That obviously causes trouble if the mxArray data is needed again after it is used by by Octave in the octave_value object as was the case in the example (the rhs data used after calling mexCallMATLAB).

I'll create a proper (set of) changeset(s) soon and commit on stable.




(file #52896)

John W. Eaton <jwe>
Group administrator
Mon 17 Jan 2022 05:42:47 AM UTC, comment #8: 

I bisected on the default branch and not surprisingly, it is this change that appears to be causing the trouble:


# User John W. Eaton <jwe@octave.org>
# Date 1633116351 14400
#      Fri Oct 01 15:25:51 2021 -0400
# Node ID b00ff462e0f291bca599b4c8607080a001075dc9
# Parent  f3ffb4596bd86d583358129735bff443b9b4b6d8
improve efficiency of mxArray -> octave_value conversion


I haven't had a chance to determine what I did wrong but will look at this before the 7.1.0 release.

John W. Eaton <jwe>
Group administrator
Thu 13 Jan 2022 05:42:15 PM UTC, comment #7: 

Markus: I suppose we could just revert to always copying unless std::pmr::polymorphic_allocator is available.  That's safe, and if you want speed it is at least possible.

I'll try to take a look at the other issue with mexCallMATLAB.

John W. Eaton <jwe>
Group administrator
Thu 13 Jan 2022 04:41:13 PM UTC, comment #6: 

I attach a minimal test case for the invalid write memory that I encounter with Dynare.

The file bug61813.c must be compiled with mkoctfile -mex, and .m file must resid in the same directory.

Output of Octave 7.0.90 within Valgrind (compiled with -enable-std-pmr-polymorphic-allocator):

==3924592== Invalid write of size 8
==3924592==    at 0x70ED199: mexFunction (bug61813.c:12)
==3924592==    by 0x5B0F42A: call_mex(octave_mex_function&, octave_value_list const&, int) (mex.cc:4769)
==3924592==    by 0x555123F: octave_mex_function::execute(octave::tree_evaluator&, int, octave_value_list const&) (ov-mex-fcn.cc:112)
==3924592==    by 0x55162B1: octave_function::call(octave::tree_evaluator&, int, octave_value_list const&) (ov-fcn.cc:57)
==3924592==    by 0x56CDB30: octave::tree_identifier::evaluate_n(octave::tree_evaluator&, int) (pt-id.cc:130)
==3924592==    by 0x56CE3D0: octave::tree_identifier::evaluate(octave::tree_evaluator&, int) (pt-id.h:98)
==3924592==    by 0x56AF154: octave::tree_evaluator::visit_statement(octave::tree_statement&) (pt-eval.cc:3766)
==3924592==    by 0x56DCCA5: octave::tree_statement::accept(octave::tree_walker&) (pt-stmt.h:124)
==3924592==    by 0x56AF62E: octave::tree_evaluator::visit_statement_list(octave::tree_statement_list&) (pt-eval.cc:3851)
==3924592==    by 0x54607C1: octave::tree_statement_list::accept(octave::tree_walker&) (pt-stmt.h:201)
==3924592==    by 0x56A5097: octave::tree_evaluator::eval(std::shared_ptr<octave::tree_statement_list>&, bool) (pt-eval.cc:980)
==3924592==    by 0x56A4709: octave::tree_evaluator::repl() (pt-eval.cc:801)
==3924592==  Address 0xf158410 is 0 bytes inside a block of size 8 free'd
==3924592==    at 0x484217B: free (vg_replace_malloc.c:872)
==3924592==    by 0x5B0BE86: xfree(void*) (mex.cc:299)
==3924592==    by 0x5B14A59: mx_memory_resource::do_deallocate(void*, unsigned long, unsigned long) (mex.cc:360)
==3924592==    by 0x50CB067: std::pmr::memory_resource::deallocate(void*, unsigned long, unsigned long) (memory_resource:111)
==3924592==    by 0x50D150F: std::pmr::polymorphic_allocator<double>::deallocate(double*, unsigned long) (memory_resource:186)
==3924592==    by 0x50D13FC: std::allocator_traits<std::pmr::polymorphic_allocator<double> >::deallocate(std::pmr::polymorphic_allocator<double>&, double*, unsigned long) (alloc_traits.h:345)
==3924592==    by 0x50D12D7: Array<double, std::pmr::polymorphic_allocator<double> >::ArrayRep::deallocate(double*, unsigned long) (Array.h:207)
==3924592==    by 0x50D1209: Array<double, std::pmr::polymorphic_allocator<double> >::ArrayRep::~ArrayRep() (Array.h:187)
==3924592==    by 0x50D1017: Array<double, std::pmr::polymorphic_allocator<double> >::~Array() (Array.h:356)
==3924592==    by 0x5B1CEBD: octave_value mxArray_base_full::fp_to_ov<double>(dim_vector const&) const (mex.cc:2149)
==3924592==    by 0x5B18754: mxArray_base_full::as_octave_value() const (mex.cc:2084)
==3924592==    by 0x5B196AB: mxArray_separate_full::as_octave_value() const (mex.cc:2411)
==3924592==  Block was alloc'd at
==3924592==    at 0x483F7B5: malloc (vg_replace_malloc.c:381)
==3924592==    by 0x5B0BE37: xmalloc(unsigned long) (mex.cc:270)
==3924592==    by 0x5B1C0B7: mex::malloc_unmarked(unsigned long) (mex.cc:3414)
==3924592==    by 0x5B1C178: mex::calloc_unmarked(unsigned long, unsigned long) (mex.cc:3441)
==3924592==    by 0x5B0CF33: mxArray::calloc(unsigned long, unsigned long) (mex.cc:3686)
==3924592==    by 0x5B17C13: mxArray_base_full::mxArray_base_full(bool, mxClassID, double) (mex.cc:1653)
==3924592==    by 0x5B18FAC: mxArray_separate_full::mxArray_separate_full(mxClassID, double) (mex.cc:2321)
==3924592==    by 0x5B0CB11: mxArray::create_rep(bool, mxClassID, double) (mex.cc:3287)
==3924592==    by 0x5B0C169: mxArray::mxArray(bool, mxClassID, double) (mex.cc:3164)
==3924592==    by 0x5B0D86C: mxCreateDoubleScalar (mex.cc:3895)
==3924592==    by 0x70ED15C: mexFunction (bug61813.c:7)
==3924592==    by 0x5B0F42A: call_mex(octave_mex_function&, octave_value_list const&, int) (mex.cc:4769)


Essentially, it seems that input arguments passed to mexCallMATLAB() are deallocated when the call returns. This was not the case with Octave 6 (and MATLAB does not do that either).

Octave does not crash for me in that simple case, but it’s easy to see how it can lead to crashes in more complex scenarios.

P.S.: Feel free to rename this bug report, since the title may no longer be relevant.


(file #52661, file #52662)

Sébastien Villemot <svillemot>
Thu 13 Jan 2022 04:11:15 PM UTC, comment #5: 

I don't "like" that we use `delete []` on memory that was `malloc`-ed. But some basic tests in the other report indicated that it is likely harmless (at least on GNU/Linux and Windows).
Tbh, I'm not completely convinced that it is always save though...

CC'ing jwe: Should we revert to the less efficient memory transfer for all types? That is only if configured with `--disable-std-pmr-polymorphic-allocator` (the default).
I can see how the current implementation makes bug tracking awkward...

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Jan 2022 04:08:15 PM UTC, comment #4: 

I have recompiled Octave 7.0.90 with --enable-std-pmr-polymorphic-allocator. The present issue indeed disappears.

However I still have the crash when running Dynare, essentially in a context where mexCallMATLAB() is used from within a MEX file. The memory errors (invalid reads, writes and free) also seem to involve the Array constructors/destructors.

I’ll try to build a minimal testcase.

Sébastien Villemot <svillemot>
Thu 13 Jan 2022 03:49:21 PM UTC, comment #3: 

I found this problem as I was debugging a crash of Octave 7.0.90 when running Dynare. This is the first memory error that appears in the Valgrind log, so my assumption was that it is the root of the crash, but I may be wrong. There are other kinds of memory errors that appear afterwards (invalid memory writes).

So the crash definitely happens outside of Valgrind, but may be unrelated to the present issue.

If you think this memory problem is harmless, then I’m going to dig further into the other memory problems.

I only tested this on GNU/Linux.

Sébastien Villemot <svillemot>
Thu 13 Jan 2022 03:47:36 PM UTC, comment #2: 

Alternatively, this could probably be avoided by configuring with `--enable-std-pmr-polymorphic-allocator`.
However, that requires C++17 for Octave's API. So, all Octave packages or user C++ code would need to be compatible and be compiled with C++17 (or newer).

Also `std::pmr::polymorphic_allocator` is currently not available with libc++.

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Jan 2022 03:41:46 PM UTC, comment #1: 

This looks very similar to bug #61472.

In that report, it was acted upon the assumption that this is only ever an issue for complex type arrays.
Does this example also cause issues when it is run without Valgrind?
Do you know of platforms where this causes actual issues with real type arrays?

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Jan 2022 03:31:35 PM UTC, original submission:  

Hi,

I attach a very basic MEX file that triggers a memory management bug (incorrect memory free). This is a regression in 7.0.90.

The source has to be compiled with:

mkoctfile -mex mexmembug.c


Then, launch Octave within Valgrind:

valgrind octave-cli


When the Octave prompt appears, run:

n = mexmembug;


Then Valgrind prints the following:

==3733875== Mismatched free() / delete / delete []
==3733875==    at 0x484299B: operator delete(void*, unsigned long) (vg_replace_malloc.c:935)
==3733875==    by 0x50A5F28: __gnu_cxx::new_allocator<double>::deallocate(double*, unsigned long) (new_allocator.h:145)
==3733875==    by 0x50A5E25: std::allocator_traits<std::allocator<double> >::deallocate(std::allocator<double>&, double*, unsigned long) (alloc_traits.h:496)
==3733875==    by 0x50A5CC1: Array<double, std::allocator<double> >::ArrayRep::deallocate(double*, unsigned long) (Array.h:207)
==3733875==    by 0x50A5BB8: Array<double, std::allocator<double> >::ArrayRep::~ArrayRep() (Array.h:187)
==3733875==    by 0x50A59C7: Array<double, std::allocator<double> >::~Array() (Array.h:356)
==3733875==    by 0x5AF2580: octave_value mxArray_base_full::fp_to_ov<double>(dim_vector const&) const (mex.cc:2177)
==3733875==    by 0x5AEDE06: mxArray_base_full::as_octave_value() const (mex.cc:2084)
==3733875==    by 0x5AEED5D: mxArray_separate_full::as_octave_value() const (mex.cc:2411)
==3733875==    by 0x5AE1FDD: mxArray::as_octave_value() const (mex.cc:3242)
==3733875==    by 0x5AE1F18: mxArray::as_octave_value(mxArray const*, bool) (mex.cc:3236)
==3733875==    by 0x5AE4CAB: call_mex(octave_mex_function&, octave_value_list const&, int) (mex.cc:4785)
==3733875==  Address 0x4705de10 is 0 bytes inside a block of size 8 alloc'd
==3733875==    at 0x483F7B5: malloc (vg_replace_malloc.c:381)
==3733875==    by 0x5AE1617: xmalloc(unsigned long) (mex.cc:270)
==3733875==    by 0x5AF1769: mex::malloc_unmarked(unsigned long) (mex.cc:3414)
==3733875==    by 0x5AF182A: mex::calloc_unmarked(unsigned long, unsigned long) (mex.cc:3441)
==3733875==    by 0x5AE2713: mxArray::calloc(unsigned long, unsigned long) (mex.cc:3686)
==3733875==    by 0x5AED2C5: mxArray_base_full::mxArray_base_full(bool, mxClassID, double) (mex.cc:1653)
==3733875==    by 0x5AEE65E: mxArray_separate_full::mxArray_separate_full(mxClassID, double) (mex.cc:2321)
==3733875==    by 0x5AE22F1: mxArray::create_rep(bool, mxClassID, double) (mex.cc:3287)
==3733875==    by 0x5AE1949: mxArray::mxArray(bool, mxClassID, double) (mex.cc:3164)
==3733875==    by 0x5AE304C: mxCreateDoubleScalar (mex.cc:3895)
==3733875==    by 0x709B135: mexFunction (mexmembug.c:10)
==3733875==    by 0x5AE4C0A: call_mex(octave_mex_function&, octave_value_list const&, int) (mex.cc:4769)



Sébastien Villemot <svillemot>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #52914:  sparse-mx-2-ov-diffs.txt added by jwe (1KiB - text/plain)
file #52896:  mx-2-ov-alloc-diffs.txt added by jwe (8KiB - text/plain)
file #52661:  bug61813.c added by svillemot (353B - text/x-csrc)
file #52662:  bug61813_fun.m added by svillemot (52B - text/x-objcsrc)
file #52659:  mexmembug.c added by svillemot (254B - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by svillemot (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-03-02 jwe StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2022-02-23 jwe Attached File- Added sparse-mx-2-ov-diffs.txt, #52914
        StatusFixed In Progress
        Open/ClosedClosed Open
    2022-02-22 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-02-21 jwe StatusPatch Submitted Ready For Test
    2022-02-21 jwe Attached File- Added mx-2-ov-alloc-diffs.txt, #52896
        StatusNone Patch Submitted
    2022-01-13 svillemot Attached File- Added bug61813.c, #52661
        Attached File- Added bug61813_fun.m, #52662
    2022-01-13 mmuetzel Carbon-Copy- Added jwe
    2022-01-13 svillemot Attached File- Added mexmembug.c, #52659

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code