bugGNU Octave - Bugs: bug #60334, Embedded Octave crashes on exit in...

 
 

bug #60334: Embedded Octave crashes on exit in octave::interpreter::~interpreter()

Submitter:  None
Submitted:  Sat 03 Apr 2021 05:46:34 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Octave User Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 6.2.0
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 21 Apr 2021 12:16:11 AM UTC, comment #29: 

This works for me.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Tue 20 Apr 2021 08:57:56 PM UTC, comment #28: 

Instead of the change I proposed earlier, I pushed the following changeset on stable and merged with default.

http://hg.savannah.gnu.org/hgweb/octave/rev/841ca9987302

For the default branch I'll try to look at further changes in the way we create and delete the interpreter object.

John W. Eaton <jwe>
Group administrator
Tue 20 Apr 2021 06:32:40 PM UTC, comment #27: 

OK, I started to make this change and it is fine for the embedded use case but not so simple with the way the way the interpreter is connected to the GUI in Octave 6.

John W. Eaton <jwe>
Group administrator
Tue 20 Apr 2021 05:12:32 PM UTC, comment #26: 

Yes, I'll also mark interpreter::shutdown as deprecated in Octave 7.

John W. Eaton <jwe>
Group administrator
Tue 20 Apr 2021 05:04:36 PM UTC, comment #25: 

Now that shutdown() is a no-op function, should we mark it as deprecated on the development branch and put it in the NEWS file that will be removed in Octave 9.0?

Rik <rik5>
Group administrator
Tue 20 Apr 2021 04:50:47 PM UTC, comment #24: 

OK, thanks for confirming.  I'll take care of adding a commit message and pushing on stable.

John W. Eaton <jwe>
Group administrator
Tue 20 Apr 2021 04:30:12 PM UTC, comment #23: 

I tried the patch and it works fine on dev.

I also tried on stable and it works there too.  It was a little bit more work because the patch had a .rej file, but it was still trivial.

Since the API doesn't change (there is still a shutdown() function in liboctinterp even though it now does nothing), I would vote for applying the patch to stable and merging with default.  The file embedded.cc in examples/code should also be updated to remove the call to shutdown() which is no longer necessary.


Rik <rik5>
Group administrator
Tue 20 Apr 2021 01:48:24 AM UTC, comment #22: 

I changed the destructor to be


interpreter::~interepreter (void)
{
  shutdown ();

  delete m_gh_manager;
}


and I only see the warnings if shutdown is called twice.  So it seems that it should be OK now to call shutdown from the destructor if we prevent the actions from being performed more than once.  But maybe it would just be easier to move the shutdown actions to the destructor and make interpreter::shutdown do nothing (for now; we can deprecate and remove it later).

The attached patch works for me.  As I said, I thought that doing this caused trouble for me in the past but I don't know why.  If some situation shows up that does cause trouble with the destructor arranged this way, then we can try to fix the real problem.  If you agree, go ahead and push the change or let me know and I will.


(file #51291)

John W. Eaton <jwe>
Group administrator
Mon 19 Apr 2021 10:58:08 PM UTC, comment #21: 

I can verify that changing the interpreter destructor to this gets rid of any segfault.


  interpreter::~interpreter (void)
  {
    m_symbol_table.cleanup (); // FIX invalid read access to m_cdef_manager
    delete m_gh_manager;
  }


Using a call to shutdown() in the destructor did not work as well  as the symbol table cleanup.  Octave didn't seem to crash, but it printed these two message at exit


error: ignoring const execution_exception& while preparing to exit
error: ignoring const execution_exception& while preparing to exit


So, there does seem a way out of this.  The question is whether it is enough.

Octave does call m_symbol_table.cleanup () from within the shutdown() function.  So maybe the resolution is to call shutdown() from the interpreter destructor, but re-order the sequence of calls within shutdown().

Rik <rik5>
Group administrator
Mon 19 Apr 2021 10:47:09 PM UTC, comment #20: 

Because of the problem reported in bug #60420, I threw down a bookmark and grafted the latest changeset on to a new lightweight branch.  Now, I can build and run the stand-alone code in embedded.cc fine.

Rik <rik5>
Group administrator
Mon 19 Apr 2021 09:20:56 PM UTC, comment #19: 

That's happening because of an error in initialization that was introduced with the changes for the new command window widget.  I fixed that problem here:

http://hg.savannah.gnu.org/hgweb/octave/rev/97238418c028

and now the crash occurs in destructor for the interpreter object (as I think it was before).

John W. Eaton <jwe>
Group administrator
Mon 19 Apr 2021 09:02:43 PM UTC, comment #18: 

I made a version of Octave compiled with debugging symbols and it is slightly more informative.


#0  0x00007ffff6ba8c7b in octave::cmdline_options::cmdline_options (this=0x7fffffffc880)
    at libinterp/octave.h:50
#1  0x00007ffff6ba8fe9 in octave::application::options (this=0x0) at libinterp/octave.h:273
#2  0x00007ffff75c4358 in octave::interpreter::initialize (this=0x7fffffffccd0)
    at libinterp/corefcn/interpreter.cc:733
#3  0x00007ffff75c4a91 in octave::interpreter::execute (this=0x7fffffffccd0)
    at libinterp/corefcn/interpreter.cc:913
#4  0x000055555555871a in main () at embedded.cc:42


Rik <rik5>
Group administrator
Mon 19 Apr 2021 08:56:46 PM UTC, comment #17: 

Compiled the test example embedded.cc and ran under gdb.  The backtrace is


#0  0x00007ffff76473bc in octave::cmdline_options::cmdline_options(octave::cmdline_options const&) ()
   from /home/rik/local/lib/octave/7.0.0/liboctinterp.so.8
#1  0x00007ffff763e265 in octave::interpreter::initialize() ()
   from /home/rik/local/lib/octave/7.0.0/liboctinterp.so.8
#2  0x00007ffff76469a6 in octave::interpreter::execute() ()
   from /home/rik/local/lib/octave/7.0.0/liboctinterp.so.8
#3  0x000055555555871a in main () at embedded.cc:42


Seems like the cmdline_options constructor is the source.

Rik <rik5>
Group administrator
Thu 15 Apr 2021 04:50:55 PM UTC, comment #16: 

Using LD_LIBRARY_PATH I can switch which liboctinterp the embedded executable uses.  The exact same executable running against dev libraries segfaults, but against 6.2 libries works.

Rik <rik5>
Group administrator
Thu 15 Apr 2021 04:47:25 PM UTC, comment #15: 

Just to be sure, I used the stable version (6.2.0) and embedded.cc compiles and runs correctly.  So there is now a regression somewhere in the interpreter shutdown.

Rik <rik5>
Group administrator
Thu 15 Apr 2021 04:44:01 PM UTC, comment #14: 

I tried to do some experimentation, but I now find that the default example we ship in code/examples/embedded.cc segfaults every time.  This code already includes the call to shutdown() so that is bad news.  This is the example that is also referenced in the manual.

Rik <rik5>
Group administrator
Sat 10 Apr 2021 06:43:25 AM UTC, comment #13: 

I can confirm, that calling interpreter.shutdown() before ~interpreter() solves the issue. Actually, interpreter.shutdown() contains also a function call to m_symbol_table.cleanup() as proposed in the patch submitted.

However, I would prefer a solution as proposed in comment #12 because a variable could go out of scope for a number of reasons (e.g. because an exception was thrown even within a constructor, because of a return, break or goto statement, for a static variable because exit was called, ...). The C++ language allows us to handle all those cases conveniently just by writing a proper destructor.

Anonymous
Thu 08 Apr 2021 08:37:22 PM UTC, comment #12: 

Maybe we get lucky and just adding


interpreter.shutdown ()


as the very first thing in the interpreter destructor will be enough.

Rik <rik5>
Group administrator
Thu 08 Apr 2021 07:50:52 PM UTC, comment #11: 

Rik: I agree with what you say about expecttations.  I'm not sure what the reason was, but I recall having trouble with performing the shutdown actions in the destructor and (perhaps incorrectly) concluding that by the time the destructor was called it was already too late for some of those actions.  Maybe it was not too late, but they were just happening in the wrong order.  I'll try to take another look at it.

John W. Eaton <jwe>
Group administrator
Thu 08 Apr 2021 02:54:38 PM UTC, comment #10: 

Summarizing what I understand from comment #9, there are another two possible paths to resolution

1) Change documentation to note that interpreter.shutdown () must be called for embedded interpreters.

Incidentally, the example code that Octave distributes for this use case is the file embedded.cc and the final lines of that file are


  // Shutdown the interpreter which cleanly releases all memory.
  interpreter.shutdown ();


So, if users just copy&paste the sample code and then build from there this will work.

2) Add interpreter.shutdown () to the destructor for an intrepreter object.

Off hand, this would seem to conform more to programmer's expectations.  An interpreter object is created easily with


octave::interpreter interpreter;


and a programmer would normally expect that when an object goes out of scope it naturally takes care of cleaning itself up.  Just to push the point, if I write


std::string mystring ("Hello World");


I don't expect to have to write


mystring.clear ();


at the bottom of the function that instantiates the variable.

Rik <rik5>
Group administrator
Wed 07 Apr 2021 07:52:37 PM UTC, comment #9: 

I think the crash can also be avoided by adding


interpreter.shutdown ();


to your OctaveInterface destructor.  That will perform shutdown actions.

Maybe we should be calling that from the interpreter destructor so that it happens automatically when the interpreter object goes out of scope, but I recall having trouble with that the last time I looked at it.  Sorry that these interfaces keep changing.  I hope we can arrive at something that is stable and works as expected before the next release.

John W. Eaton <jwe>
Group administrator
Wed 07 Apr 2021 07:49:55 PM UTC, comment #8: 

Dear Mr. Eaton,

To make it easier for you, I have provided a very simple example which triggers that bug.
Just unpack embedded_octave.tar.bz2, edit the Makefile to get the right Octave version and run make check.

With octave-5.2.0 valgrind reports no errors, but with the development version I get again that invalid read of size 8 in ~symbol_table.


(file #51209)

Anonymous
Wed 07 Apr 2021 07:07:14 PM UTC, comment #7: 

I see, there seems to be another issue with the development version of Octave which is related to the number of input arguments.

But maybe you can try also this command:
`./mbdyn -f ../modules/module-octave/demo/octave_const_law.mbdyn`

If I run it, I get this output:

MBDyn - MultiBody Dynamics develop
configured on Apr  5 2021 at 19:24:37

Copyright 1996-2017 (C) Paolo Mantegazza and Pierangelo Masarati,
Dipartimento di Ingegneria Aerospaziale <http://www.aero.polimi.it/>
Politecnico di Milano                   <http://www.polimi.it/>

MBDyn is free software, covered by the GNU General Public License,
and you are welcome to change it and/or distribute copies of it
under certain conditions.  Use 'mbdyn --license' to see the conditions.
There is absolutely no warranty for MBDyn.  Use "mbdyn --warranty"
for details.

reading from file "/home/reinhard/work/mbdyn-git/modules/module-octave/demo/octave_const_law.mbdyn"
configure with --enable-multithread for multithreaded assembly
Creating scalar solver with Umfpack linear solver
Reading Structural(1)
Reading Structural(2)
Reading Structural(3)
Reading Abstract(1)
Reading Body(1)
Reading Body(2)
Reading Joint(1)
Reading Genel(1)
Reading Genel(2)
Reading Joint(2)
Reading Joint(3)
Reading Force(1)
Reading Force(2)
Reading Force(3)
mboct: embed octave is enabled!
Step  2060        1.0004        0.0005    1   2.14611e-18             0 0 1
End of simulation at time 1.0004 after 2060 steps;
output in file "/tmp/test_octave"
total iterations: 2060
total Jacobian matrices: 2092
total error: 7.32396e-05
Segmentation fault (core dumped)


An if I run it through valgrind, I get the same invalid read access:

==3313== Invalid read of size 8
==3313==    at 0x53A36C8: compare (basic_string.h:2855)
==3313==    by 0x53A36C8: operator< <char, std::char_traits<char>, std::allocator<char> > (basic_string.h:6229)
==3313==    by 0x53A36C8: operator() (stl_function.h:386)
..............
==3313==    by 0x5A25919: ~_Rb_tree (stl_tree.h:995)
==3313==    by 0x5A25919: ~map (stl_map.h:300)
==3313==    by 0x5A25919: ~symbol_table (symtab.h:69)
==3313==    by 0x5A25919: octave::interpreter::~interpreter() (interpreter.cc:646)
==3313==    by 0x52BDEC: oct::OctaveInterface::~OctaveInterface() (module-octave.cc:938)


Anonymous
Wed 07 Apr 2021 06:01:56 PM UTC, comment #6: 

I tried the instructions in comment #5.  When I tried to use a build of the current development version (--with-mkoctfile=/usr/local/octave/dev/bin/mkoctfile) the mbdyn build failed with missing symbol errors that I'm not sure I understand.

When I tried again without using the --with-mkoctfile configure option, the build succeeded (I have Octave 6.2.0 installed from the Debian package) but running the example does not fail with a segfault:


bash> ./mbdyn -f ../modules/module-octave/demo/octave_element.mbdyn

MBDyn - MultiBody Dynamics develop
configured on Apr  6 2021 at 08:18:36

Copyright 1996-2017 (C) Paolo Mantegazza and Pierangelo Masarati,
Dipartimento di Ingegneria Aerospaziale <http://www.aero.polimi.it/>
Politecnico di Milano                   <http://www.polimi.it/>

MBDyn is free software, covered by the GNU General Public License,
and you are welcome to change it and/or distribute copies of it
under certain conditions.  Use 'mbdyn --license' to see the conditions.
There is absolutely no warranty for MBDyn.  Use "mbdyn --warranty"
for details.

reading from file "../modules/module-octave/demo/octave_element.mbdyn"
configure with --enable-multithread for multithreaded assembly
Creating scalar solver with Naive linear solver
Reading Structural(1)
Reading Structural(2)
Reading Structural(3)
Reading Structural(4)
Reading Parameter(1)
Reading Parameter(2)
Reading Body(1)
Reading Body(2)
Reading Joint(1)
Reading Joint(2)
Reading Force(1)
Reading Force(2)
Reading Force(3)
Reading Force(4)
Reading LoadableElement(1)
An error occurred during the execution of MBDyn; aborting...


Although your patch may avoid the crash for you, I'm hesitant to install it without understanding why the crash is happening.

John W. Eaton <jwe>
Group administrator
Tue 06 Apr 2021 10:55:26 AM UTC, comment #5: 

It is not that simple to reproduce the issue. You have to build the MBDyn software with support for GNU/Octave:


git clone -b develop https://public.gitlab.polimi.it/DAER/mbdyn.git

cd mbdyn
./bootstrap.sh
./configure --enable-octave --with-static-modules --with-mkoctfile="${MKOCTFILE}"
make
cd mbdyn
./mbdyn -f ../modules/module-octave/demo/octave_element.mbdyn


In the code above, "${MKOCTFILE}" is the full path of your specific version of Octave.

Anonymous
Mon 05 Apr 2021 06:04:49 PM UTC, comment #4: 

Can you please provide a complete example that will demonstrate the problem?  What exactly do you have to do to cause the crash to happen?

John W. Eaton <jwe>
Group administrator
Mon 05 Apr 2021 05:52:48 PM UTC, comment #3: 

Dear Rik,

I have checked the default branch and there is the same problem as you can see from valgrind's log-file (e.g. invalid read access of size 8 in ~symbol_table).


(file #51195)

Anonymous
Mon 05 Apr 2021 03:24:39 PM UTC, comment #2: 

Do you have the ability to build Octave from development sources?  I'ts pretty easy on Linux.

There has been quite a bit of re-factoring on the development branch around the shutdown sequence for Octave, and I need to verify whether it has been fixed there already.  If it has, then your one-line fix is probably fine for the stable branch (6.2.0), but shouldn't be propagated to the development branch.

Or the development branch also has this issue, in which case the patch should be applied everywhere (stable and development).

Rik <rik5>
Group administrator
Sun 04 Apr 2021 08:03:28 AM UTC, comment #1: 

Dear GNU/Octave developers,

I have found another solution to solve the issue which requires only a single additional line in "interpreter.cc".

Best regards,
Octave User

(file #51187)

Anonymous
Sat 03 Apr 2021 05:46:34 PM UTC, original submission:  

Dear GNU/Octave developers,

If I'm using Octave 6.2.1 in module-octave of MBDyn (https://public.gitlab.polimi.it/DAER/mbdyn), I get a SIGSEGV on exit in "octave::interpreter::~interpreter()". If MBDyn is compiled with Octave 5.2.0, everything works find.

The SIGSEGV happens when the destructor of the symbol_table is called, because the cdef_manager is destroyed before the symbol_table and the symbol_table still references objects from the cdef_manager.
You can verify that in the attached log file from valgrind.

This issue can be fixed by calling the destructor of the symbol table before the constructor of the cdef_manager. I have attached a patch for that purpose.
When applying that patch, Octave's test suite shows no regressions.

Best regards,
Octave User

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51291:  interp-shutdown-diffs.txt added by jwe (12KiB - text/plain)
file #51209:  embedded_octave.tar.bz2 added by None (896B - application/x-bzip)
file #51195:  valgrind.log added by None (1MiB - text/x-log)
file #51186:  valgrind.log added by None (1MiB - text/x-log)

 

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 rik5 (Posted a comment)
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by None (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-04-21 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2021-04-20 rik5 StatusPatch Submitted Patch Reviewed
    2021-04-20 jwe Attached File- Added interp-shutdown-diffs.txt, #51291
    2021-04-07 None Attached File- Added embedded_octave.tar.bz2, #51209
    2021-04-05 None Attached File- Added valgrind.log, #51195
    2021-04-05 siko1056 StatusNone Patch Submitted
    2021-04-04 None Attached File- Added embedded-octave-fix-SIGSEGV-on-exit.patch, #51187
    2021-04-03 None Attached File- Added embedded-octave-fix-SIGSEGV-on-exit.patch, #51185
        Attached File- Added valgrind.log, #51186

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code