bugGNU Octave - Bugs: bug #61472, AddressSanitizer crash in MEX tests

 
 

bug #61472: AddressSanitizer crash in MEX tests

Submitted by:  Arun Giridhar <arungiridhar>
Submitted on:  Sat 13 Nov 2021 11:21:14 AM UTC  
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  7 - High Item Group:  Build Failure
Status:  None Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  7.0.90 Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Mon 06 Dec 2021 07:34:17 PM UTC, comment #30: 

Markus: Yes, the changes you made did help. thanks.  I think it was fixing the declarations for octave_int<T>.  I have some more pending changes for range<T>, MArray<T>, and intNDArray<T>.  There are probably a few others that also need to be fixed but none of these other changes appear to be necessary to fix the visibility problems.

I'll look at enabling the polymorphic allocator only if we are using a compiler that claims to be C++17 actually supports the feature and otherwise falling back on copying complex objects otherwise.  I can also make copying an option for non-class objects, though I think it should be OK to mix malloc with delete for those.  If it causes trouble on actual systems, users can disable it.

John W. Eaton <jwe>
Project Administrator
Mon 06 Dec 2021 04:58:39 PM UTC, comment #29: 

Oh, I think I did attach them but then I previewed my reply and then submitted it.  Does that lose the attachments?  Hmm.  Anyway, I'm attaching the two files here.

(file #52441, file #52442)

John W. Eaton <jwe>
Project Administrator
Mon 06 Dec 2021 04:10:08 PM UTC, comment #28: 

@jwe: Did you intend to attach some files to your last comment?

Wrt the visibility issue: Does that still happen with a current tip? Some of the changes for bug #59820 might have improved the situation...

Markus Mützel <mmuetzel>
Project Member
Mon 06 Dec 2021 03:32:30 PM UTC, comment #27: 

After looking at this a little more, I'm not sure what the best solution is.

We only mix malloc with delete in the case of returning array data from MEX functions.  Those arrays are either double/float or pairs of double/float used to represent complex numbers (and then, we only mix malloc/delete when using the new interleaved complex storage layout that is enabled when using the -R2018a option for compiling MEX files).

With double data, there are no destructors to call.  With complex, the destructors don't actually do anything.  I know it works for double arrays because I've tested that.  But it fails for complex.  Did I not test that?!?  I guess not.  I'm attaching a simple mex file to demonstrate.  Using stable Octave without my proposed changes, try

octave> mkoctfile -mex -R2018a freetest.c
octave> x = freetest ();
octave> clear x  ## EXPECT error and crash

Note that it should work if you don't use interleaved complex data (omit the -R2018a option or use -R2017b instead).

I'm also attaching a simpler stand-alone program that demonstrates the problem.  When I run it, I see:

$ g++ -O2 vec-dtor.cc
$ ./a.out
new + delete[] for double: ok
malloc + delete[] for double: ok
new + delete[] for complex: ok
malloc + delete for complex: ok
malloc + delete[] for complex: free(): invalid pointer
Aborted

If we could find a simple workaround for the visibility problem, I would have no objection to using the polymorphic allocator when using a compiler that supports it.

Otherwise, I think the best solution for version 7 is to only allow malloc and delete to be mixed when returning real-valued arrays from MEX functions.  For complex data, we can just go back to copying data even when using interleaved complex storage.

John W. Eaton <jwe>
Project Administrator
Mon 06 Dec 2021 11:10:14 AM UTC, comment #26: 

By the looks of it, a compiler (or its preprocessor) can claim to be C++17 complete even if the C++ standard library it will be using doesn't implement all features.

Anyhow, I'm with jwe on this. It looks like a proper fix will be quite intrusive. We'd need to raise the minimum requirements quite significantly (libstdc++ version 9 or newer, no support for libc++ currently).
Alternatively, we'd need to introduce a new dependency on the boost libraries. (It needs confirmation if this is an actual option.)
In any case, it looks like the changes will be too significant to squeeze into Octave 7 without delaying its release (possibly substantially).

For Octave 7, should we just leave it as it is and be aware that the mex interface might work unexpectedly with certain combinations of compiler versions/C++ library versions/platforms/... at certain operations? As far as we currently can tell, a normal user might not notice anything about this on most (if not all?) platforms because the C++ standard libraries probably implement `delete` on top of `free`...

If we are using `delete[]` though, anything might happen ranging from memory leaks to freeing a huge amount of memory (resulting in loads of dangling references or crashing immediately). This is because `delete[]` seems to read the memory in front of where the pointer points to find how much memory to actually free. In general, that could be anything for malloc-ed data.
See e.g. this for how it is implemented in MS compilers (but I'm assuming other compilers don't do it much differently):
https://devblogs.microsoft.com/oldnewthing/20040203-00/?p=40763

IIUC, the oct interface isn't affected by any of this.

Markus Mützel <mmuetzel>
Project Member
Mon 06 Dec 2021 02:02:52 AM UTC, comment #25: 

If currently C++17 cannot be adopted there are multiple polymorphic allocator implementations on web compatible with C++11 including:

1. https://github.com/phalpern/CppCon2017Code

It's Author proposed polymorphic allocators to standard.

2. http://man.hubwiz.com/docset/Boost.docset/Contents/Resources/Documents/boost/doc/html/container/cpp_conformance.html

Boost implementation.

Anonymous
Sun 05 Dec 2021 11:19:31 PM UTC, comment #24: 

@Dmitri: Markus had a technique for running only the MEX test here: https://octave.discourse.group/t/running-and-disabling-bists/1903/2 which is reproduced here:

make test/mex/bug_51725.mex
./run-octave --eval "addpath('../test/mex', 'test/mex'); test ../test/mex/bug-51725.tst"

ASAN definitely reports memory leaks but it's not clear if they're connected to MEX (probably not, since even when ASAN crashed at the Jupyter tests just before the MEX tests it still reported memory leaks. The Jupyter leaks are listed as being a possible false positive by ASAN itself, based on Octave's use of custom stack unwinding.)

Arun Giridhar <arungiridhar>
Sun 05 Dec 2021 07:00:43 PM UTC, comment #23: 

The problem with a "normal" build might be a memory leak.
I am not sure how to run this test standalone
"test bug-54096.tst" does not work for me.
The idea is to do it many times in a loop and see if memory usage increases.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 05 Dec 2021 06:47:09 PM UTC, comment #22: 

@jwe in comment #19: Yes, changing the typedef to add the word "template", and changing INSTANTIATE_ARRAY_WITH_ALLOCATOR to INSTANTIATE_ARRAY both made Clang compile it fully without errors. I also did not need to add --disable-lib-visibility-flags  to configure. But it did give the malloc-vs-free warnings during compilation and the MEX test crashed ASAN. It might be because the #if check for C++17 works only for GCC and not Clang.

Re whether this should be fixed for 7.1 or not, it causes a problem only in ASAN checks and not in regular use (comment #2).

Arun Giridhar <arungiridhar>
Sun 05 Dec 2021 04:03:47 PM UTC, comment #21: 

Mixing `malloc` and `delete` causes undefined behavior. So, no guarantees what could or could not happen for any platform. But a quick Google search didn't show any definite examples for platforms where that actually causes trouble. (I might not have used the correct search terms though.)

Markus Mützel <mmuetzel>
Project Member
Sun 05 Dec 2021 03:43:39 PM UTC, comment #20: 

Perhaps it is best to defer fixing this problem until after version 7 is released and to make any changes related to it on the default branch?

Does mixing malloc and delete cause trouble for is in real life, at least for the systems we currently care about?  If so, then we could also change the MEX functions to fall back to copying data on those systems where it matters, at least for now.

John W. Eaton <jwe>
Project Administrator
Sun 05 Dec 2021 03:16:36 PM UTC, comment #19: 

Arun, doeas it help if you change the typedef to

typedef typename Alloc_traits::template rebind_traits<T> T_Alloc_traits;

or does clang really not support this feature even though it apparently is claiming to be a c++17 compiler (based on the definition of __cplusplus that enables this code)?

Also, there was a typo in the cset-1-array-alloc.txt file that I posted.  Instead of INSTANTIATE_ARRAY_WITH_ALLOCATOR, it should have just been INSTANTIATE_ARRAY.

John W. Eaton <jwe>
Project Administrator
Sun 05 Dec 2021 02:36:15 PM UTC, comment #18: 

Like Anonymous wrote in comment #14, libc++ (used by default when compiling with clang) doesn't implement support for polymorphic memory resources. So, that is expected to fail.
You might get more lucky with clang if you try and link to libstdc++.
See also:
https://en.cppreference.com/w/cpp/compiler_support/17

Markus Mützel <mmuetzel>
Project Member
Sun 05 Dec 2021 02:30:12 PM UTC, comment #17: 

Yes, I was compiling with GCC 11.2. With GCC, adding --disable-lib-visibility-flags makes it build properly. Make check passes for the MEX tests with that flag.

With Clang 13 errors like these show whether or not --disable-lib-visibility-flags is used. The configure flags, CFLAGS, CXXFLAGS etc are all the same as with the GCC build:

In file included from ../libinterp/dldfcn/__delaunayn__.cc:53:
../liboctave/array/Array.h:139:36: error: use 'template' keyword to treat 'rebind_traits' as a dependent template name
    typedef typename Alloc_traits::rebind_traits<T> T_Alloc_traits;
                                   ^
                                   template

Arun Giridhar <arungiridhar>
Sun 05 Dec 2021 09:39:54 AM UTC, comment #16: 

Your change e38202d3628d seems to have greatly reduced the number of warnings on the macOS runners about "direct access to global weak symbols" (see bug #59820). The remaining ones are mostly about instantiations involving `class Sparse<T>` and `class octave_int<T>` templates afaict.
Maybe if we take the same approach (dedicated headers to provide guarded forward declarations for those template classes), we could reduce the impact of that bug. Maybe that could also resolve the issue you are seeing when using the patches you provided here with enabled visibility flags.

Markus Mützel <mmuetzel>
Project Member
Sat 04 Dec 2021 11:11:10 PM UTC, comment #15: 

RE: comment #13.  I'm using g++ 11 which has -std=c++17 as the default.  I see now that it was only working for me because I happened to have configured with --disable-lib-visibility-flags.  If I remove that configure option I see the same errors as you.  I'm looking at it, but I'm not sure what the correct fix is for that.  For me, it appears to be only the Arrays of octave_{u,}int{16,32,64}.  There aren't complaints about Arrays of bool, char, float, or double values, or for octave_{u,}int8.  Whatever the difference is for those types, I'm not seeing it yet.

John W. Eaton <jwe>
Project Administrator
Sat 04 Dec 2021 08:03:35 PM UTC, comment #14: 

Polymorphic allocator. Good news! It is a big step forward. I guess it can be used to cheaply send nested structures and cells from mex to Octave also it can bring the opportunity to make Arrays much dynamic like std::vector i.e. supporting reserve, push_back and emplace_back having amortized constant complexity. Thanks!

I'm not sure but I think support of polymorphic allocator in clang libc++ is currently incomplete.

Anonymous
Sat 04 Dec 2021 07:25:30 PM UTC, comment #13: 

With the two patches, I am getting build errors like the following:

  CXXLD    src/octave-cli
  CXXLD    src/octave-gui
/usr/bin/ld: libinterp/.libs/liboctinterp.so: undefined reference to `Array<octave_int<unsigned short>, std::pmr::polymorphic_allocator<octave_int<unsigned short> > >::column(long) const'
/usr/bin/ld: libinterp/.libs/liboctinterp.so: undefined reference to `Array<octave_int<long>, std::pmr::polymorphic_allocator<octave_int<long> > >::checkelem(Array<long, std::pmr::polymorphic_allocator<long> > const&) const'
/usr/bin/ld: libinterp/.libs/liboctinterp.so: undefined reference to `Array<octave_int<unsigned int>, std::pmr::polymorphic_allocator<octave_int<unsigned int> > >::nnz() const'

I think it's trying to compile with C++11 instead of C++17. Does it pick up automatically or does it need to be forced via CXXFLAGS?

Arun Giridhar <arungiridhar>
Sat 04 Dec 2021 04:05:18 PM UTC, comment #12: 

I would prefer to avoid adding function pointers to the Array classes.  Instead, I think we should extend them with template parameters that allow for custom allocators in the same way that the standard library containers do.

I'm attaching two changesets for discussion.

The first adds a template parameter to the Array class to allow allow custom allocators and uses the allocator class to manage storage allocation for the ArrayRep object.  In this change, the default allocator is std::allocator<T> so it will work with C++11.  But if you attempt to define a custom allocator for the MEX objects using this interface, you'll run into trouble because Array<double,std::allocator<double>> is a different type from Array<double,my_special_allocator> so storing the Array with the custom allocator in an octave_value object requires modifying the octave_value class to accept a new type.  We definitely don't want that.

The second change changes the default allocator for the Array class to be std::pmr::polymorphic_allocator<T> and modifies the Array<T,Alloc>::Array(T *data, size_t n) constructor to accept a allocator object.  With this interface, we don't end up with different types when using custom allocators so no changes are required for the octave_value container.  However, the change requires C++17 so I made the change conditional.  If you don't have C++17 features, you'll continue to mix malloc/delete.

Note that these changes don't add data to the Array classes.  The allocator objects don't have data, they just provide functions.

Things that remain to be done are

  • Add allocators template parameters to other containers in Octave like Sparse<T>
  • Add allocator parameters to other constructors
  • Move the definition of the custom MEX allocator to a separate file.

To try the two attached changesets, you'll need to be at revision http://hg.savannah.gnu.org/hgweb/octave/rev/e38202d3628d or newer

(file #52426, file #52427)

John W. Eaton <jwe>
Project Administrator
Fri 03 Dec 2021 02:14:34 PM UTC, comment #11: 

Thank you, Anon. Here are Anon's changes rolled up into one hg diff (attached).

Make check does not crash for the MEX tests with these edits but ASAN indicates much heavier memory leakage than before, about 1.7 MB instead of 100 KB. I have not isolated the source yet.

(file #52421)

Arun Giridhar <arungiridhar>
Fri 03 Dec 2021 01:32:25 AM UTC, comment #10: 

Attachments are limited to 4 files. Here is the 5th. Sorry for spam:

(file #52417)

Anonymous
Fri 03 Dec 2021 01:27:38 AM UTC, comment #9: 

Attachments here missed from the last post:

(file #52413, file #52414, file #52415, file #52416)

Anonymous
Fri 03 Dec 2021 01:23:31 AM UTC, comment #8: 

Regarding comment #6:

I fixed 4 files :

liboctave/array/Array.h 
liboctave/array/Sparse.h
liboctave/array/Sparse.cc
libinterp/corefcn/mex.cc

and prepared a git diff because I'm not sure how to prepare a proper hg patch. Please feel free to use the diff or use 4 fixed files to create a patch.

Anonymous
Thu 02 Dec 2021 04:54:32 PM UTC, comment #7: 

I will work on this problem.  I think the best solution is to add an allocator class as a new template argument for the Array<T> class, similar to the way that allocators are used in the C++ standard library.

John W. Eaton <jwe>
Project Administrator
Thu 02 Dec 2021 03:23:26 PM UTC, comment #6: 

Some days ago I had tried incorporating the code from Anonymous in comment #4 but it was not clear. The variable T *m_data is part of the ArrayRep class but comment #4 adds it directly to the Array class. It was not clear which classes to modify with the custom destructor (Array or ArrayRep or something else).

@Anonymous from comment #4: if you are reading this, please consider adding a Mercurial patch.

Arun Giridhar <arungiridhar>
Thu 02 Dec 2021 02:02:59 PM UTC, comment #5: 

While certain combinations of compilers and standard library implementations might work without issues when using `delete []` on `malloc`-ed memory, that might not be the case with different combinations on other platforms. IIUC, this is undefined behavior.

We should probably better fix this before the release of Octave 7.

Markus Mützel <mmuetzel>
Project Member
Tue 23 Nov 2021 09:42:35 AM UTC, comment #4: 

Ah! Sorry, Something in the last post is wrong(using delete in lambda). Here is the corrected one:

using deleter_t = void (*)(void *);

template<typename T>
class Array
{
public:
  Array (const dim_vector& dv)
  : m_data (new T[dv.safe_numel ()])
  , m_dv (dv)
  , m_deleter ([](void *p){delete[] static_cast<T *>(p);})
  {}

  Array (T *buffer, const dim_vector& dv, deleter_t del)
  : m_data (buffer)
  , m_dv (dv)
  , m_deleter (del)
  {}

  ~Array()
  {
    m_deleter (m_data);
  }

private:
  T *m_data;
  dim_vector m_dv;
  deleter_t m_deleter;
};

// usage
double * buf = (double *) std::malloc (5 * sizeof (double));

Array<doudblr> arr (buf, dim_vector (1, 5), &std::free);

Anonymous
Tue 23 Nov 2021 06:42:44 AM UTC, comment #3: 

Hello there!
Regarding comment #1:
A custom deleter can be provided to the constructor of `Array`. The Array also needs a member of type deleter:

using deleter_t = void (*)(void *);

template<typename T>
class Array
{
public:
  Array (const dim_vector& dv)
  : m_data (new T[dv.safe_numel ()])
  , m_dv (dv)
  , m_deleter ([](void *p){::operatr delete[] (p);})
  {}

  Array (T *buffer, const dim_vector& dv, deleter_t del)
  : m_data (buffer)
  , m_dv (dv)
  , m_deleter (del)
  {}

  ~Array()
  {
    deleter (m_data);
  }

private:
  T *m_data;
  dim_vector m_dv;
  deleter_t m_deleter;
};

It can be used like this:

double * buf = (double *) std::malloc (5 * sizeof (double));

Array<doudblr> arr (buf, dim_vector (1, 5), &std::free);

Anonymous
Tue 23 Nov 2021 12:51:45 AM UTC, comment #2: 

This behavior only shows up in ASAN builds and not in regular use. Your comment indicates it does not break things elsewhere in Octave. Unless it is an indicator of future trouble (e.g. a future C++ standard makes it illegal to mix them), I feel it is probably not worth your time implementing these changes simply to placate ASAN. Thoughts? Do you think it's safe to ask ASAN to ignore the mismatch?

Arun Giridhar <arungiridhar>
Mon 22 Nov 2021 08:31:28 PM UTC, comment #1: 

When I made this change http://hg.savannah.gnu.org/hgweb/octave/rev/b00ff462e0f2 I forgot that the mxMalloc, mxCalloc, mxRealloc, and mxFree actually use the corresponding C-library functions instead of C++ new and delete operators.  Hmm.  So I guess we need to either keep track of that in the Array classes or implement these mxArray memory management functions using the C++ new and delete operators.  Or is there a better solution?

John W. Eaton <jwe>
Project Administrator
Sat 13 Nov 2021 11:21:14 AM UTC, original submission:  

There is a repeatable crash during the execution of mex/bug-51725.tst in a debug build, with AddressSanitizer complaining about "alloc-dealloc-mismatch (malloc vs operator delete [])". Does anyone else see this? Is it safe to ignore it by setting ASAN_OPTIONS="alloc_dealloc_mismatch=0"?

This is for Octave 7, hg id 6c7567bfac91.

Output attached.

Arun Giridhar <arungiridhar>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #52441:  freetest.c added by jwe (612B - text/x-csrc)
file #52442:  vec-dtor.cc added by jwe (2KiB - text/x-c++src)
file #52426:  cset-1-array-alloc.txt added by jwe (58KiB - text/plain)
file #52427:  cset-2-custom-mex-alloc.txt added by jwe (5KiB - text/plain)
file #52421:  anon_diff.patch added by arungiridhar (10KiB - text/x-patch)
file #52417:  bug61472.diff added by None (10KiB - application/octet-stream - pacht)
file #52413:  Array.h added by None (28KiB - text/plain - 4 modified files)
file #52414:  Sparse.h added by None (22KiB - text/plain - 4 modified files)
file #52415:  Sparse.cc added by None (89KiB - text/plain - 4 modified files)
file #52416:  mex.cc added by None (122KiB - text/plain - 4 modified files)
file #52267:  mex_crash added by arungiridhar (31KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    16:58 jwe Attached File- => Added freetest.c, #52441
        Attached File- => Added vec-dtor.cc, #52442
    2021-12-04 jwe Attached File- => Added cset-1-array-alloc.txt, #52426
        Attached File- => Added cset-2-custom-mex-alloc.txt, #52427
    2021-12-03 arungiridhar Attached File- => Added anon_diff.patch, #52421
    2021-12-03 None Attached File- => Added bug61472.diff, #52417
    2021-12-03 None Attached File- => Added Array.h, #52413
        Attached File- => Added Sparse.h, #52414
        Attached File- => Added Sparse.cc, #52415
        Attached File- => Added mex.cc, #52416
    2021-12-02 mmuetzel Priority5 - Normal => 7 - High
        Releasedev => 7.0.90
        Operating SystemGNU/Linux => Any
    2021-11-13 arungiridhar Attached File- => Added mex_crash, #52267

    Back to the top


    Powered by Savane 3.7