bugGNU Octave - Bugs: bug #52666, Segfault when closing a figure...

 
 

bug #52666: Segfault when closing a figure inside a "deletefcn" callback

Submitter:  Pantxo Diribarne <pantxo>
Submitted:  Thu 14 Dec 2017 07:14:05 PM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 17 Dec 2017 02:43:33 AM UTC, comment #12: 

I pushed the patch to stable here (http://hg.savannah.gnu.org/hgweb/octave/rev/13344f00c564).

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Fri 15 Dec 2017 10:17:45 PM UTC, comment #11: 

I think I tested functions like

fcn = @(h) delete (ancestor (h, "figure"));
saybye = @(h) printf("BYE BYE\n");
whyfcn = @(h) delete (h);

for "deletefcn" and didn't see any issues.

Dan Sebald <sebald>
Fri 15 Dec 2017 10:07:04 PM UTC, comment #10: 

Patch works for me.  'make check' passes.  I don't see any memory leaks using ASAN and the example from this report.

Anyone else have some ideas for testing or should I just commit it?  Since it is a segfault, I would probably push it to stable.

Rik <rik5>
Group administrator
Fri 15 Dec 2017 04:46:26 AM UTC, comment #9: 

Give the attached changeset a try.  I didn't think through whether all memory and other lists are cleaned up appropriately in this scenario, just verified that this doesn't freeze or segfault.

(file #42653)

Dan Sebald <sebald>
Fri 15 Dec 2017 04:33:26 AM UTC, comment #8: 

I've printed out a bit more:


diff --git a/libinterp/corefcn/graphics.cc b/libinterp/corefcn/graphics.cc
--- a/libinterp/corefcn/graphics.cc
+++ b/libinterp/corefcn/graphics.cc
@@ -2661,6 +2661,7 @@ gh_manager::do_free (const graphics_hand

       octave_value val = bp.get_deletefcn ();

+std::cerr << "Ex deletefcn: " << &bp << "\n";
       bp.execute_deletefcn ();

       // Notify graphics toolkit.
@@ -2798,9 +2799,11 @@ delete_graphics_object (const graphics_h
     {
       graphics_object go = gh_manager::get_object (h);

+std::cerr << "Checking if is_beingdeleted " << h.value() << "\n";
       // Don't do recursive deleting, due to callbacks
       if (! go.get_properties ().is_beingdeleted ())
         {
+std::cerr << "Passed check\n";
           graphics_handle parent_h = go.get_parent ();

           graphics_object parent_go = gh_manager::get_object (parent_h);


And I see:


octave:1> fcn = @(h) close (ancestor (h, "figure"));
octave:2> hl = line ("deletefcn", fcn)
hl = -6.3352
octave:3> gcf
ans =  1
octave:4> delete (hl)
Checking if is_beingdeleted -6.33522
Passed check
Ex deletefcn: 0x7f8b308aa790
Checking if is_beingdeleted 1
Passed check
Ex deletefcn: 0x7f8b308aa790
Ex deletefcn: 0x7f8b307ae360
Ex deletefcn: 0x7f8b30851990
Ex deletefcn: 0x7f8b30824980
Ex deletefcn: 0x7f8b3081ddc0
Ex deletefcn: 0x7f8b30811450
Ex deletefcn: 0x7f8b30771280
^C^CPress Control-C again to abort.
^Cfatal: caught signal Interrupt -- stopping myself...
attempting to save variables to 'octave-workspace'...
save to 'octave-workspace' complete
terminate called after throwing an instance of 'octave::exit_exception'
Aborted


OK, I understand now.  In the above note that on the first pass, the handle is -6.33522 which is the handle of the line deleted.  On the second pass, the handle is the figure window 1.  Now take a look at this hunk of code:


static void
delete_graphics_object (const graphics_handle& h)
{
  if (h.ok ())
    {
      graphics_object go = gh_manager::get_object (h);

std::cerr << "Checking if is_beingdeleted" << h.value() << "\n";
      // Don't do recursive deleting, due to callbacks
      if (! go.get_properties ().is_beingdeleted ())
        {
std::cerr << "Passed check\n";


The test is on whether the g.o. associated with the handle passed into the routine is being deleted, but on the second call (the one associated with the close() of the line's callback function) the handle is 1, not -6.33522, and the "beingdeleted" flag was not yet set for the g.o. associated with 1.  Hence the double free because the figure doesn't realize the first item in its list is in the process of being deleted.

Dan Sebald <sebald>
Fri 15 Dec 2017 02:43:29 AM UTC, comment #7: 

Well, it doesn't look like an infinite recursion, but there appears to be at least one double delete.  I added the following:


diff --git a/libinterp/corefcn/graphics.cc b/libinterp/corefcn/graphics.cc
--- a/libinterp/corefcn/graphics.cc
+++ b/libinterp/corefcn/graphics.cc
@@ -2661,6 +2661,7 @@ gh_manager::do_free (const graphics_hand

       octave_value val = bp.get_deletefcn ();

+std::cerr << "Ex deletefcn: " << &bp << "\n";
       bp.execute_deletefcn ();


and here is the output:


octave:1> fcn = @(h) close (ancestor (h, "figure"));
octave:2> hl = line ("deletefcn", fcn);
octave:3> delete (hl)
Ex deletefcn: 0x7f7a5c8aa5a0
Ex deletefcn: 0x7f7a5c8aa5a0
Ex deletefcn: 0x7f7a5c7ae0b0
Ex deletefcn: 0x7f7a5c8516a0
Ex deletefcn: 0x7f7a5c824690
Ex deletefcn: 0x7f7a5c81da90
Ex deletefcn: 0x7f7a5c8113d0
Ex deletefcn: 0x7f7a5c776a70
^C^CPress Control-C again to abort.
^Cfatal: caught signal Interrupt -- stopping myself...
attempting to save variables to 'octave-workspace'...
save to 'octave-workspace' complete
terminate called after throwing an instance of 'octave::exit_exception'
Aborted


At the very beginning the same object 0x7f7a5c8aa5a0 has its deletefcn run twice.  Just figure out how that can be happening, I guess.  (This is all single threaded int terms of setting that "beingdeleted" to true, correct?)

Dan Sebald <sebald>
Fri 15 Dec 2017 02:10:41 AM UTC, comment #6: 

I tried this.  I'm seeing 100% CPU as well.  This could be an infinite loop of the line deletefcn calling its parent's close, which calls the line's deletefcn, which calls its parent's close, etc.  Of course, something should break along the way in terms of freeing memory multiple times.  Perhaps it is only under the debugger when some bad memory access causes a segmentation fault.

What comes to mind is if the parent figure has a list of items it contains, it should remove the pointer for an item from that list and then delete it rather than delete it and then remove the pointer from the list.  If you follow... hold on, let me look at the code since Rik narrowed things down.

And here's a note in graphics.cc:


static void
delete_graphics_object (const graphics_handle& h)
{
[snip]
          // NOTE: free the handle before removing it from its parent's
          //       children, such that the object's state is correct when the
          //       deletefcn callback is executed
[snip]
}


So, how is it supposed to prevent recursive loop?  Well, there is this


      // Don't do recursive deleting, due to callbacks
      if (! go.get_properties ().is_beingdeleted ())


My guess would be that somehow that "is_beingdeleted" is failing...  OK, the way this works is that "beingdeleted" is a parameter in the base_properties and is/get/set control that parameters, so the


       bp.set_beingdeleted (true);


is set in some function "gh_manager::do_free".  Hmm, I suspect that set_beingdeleted() at that point might be too late in the chain of events to prevent a recursion.  Wouldn't it seem like that variable should be set immediately prior to calling the routine that delete's the g.o.?  Have to put some thought into where this should be set, and maybe if it should be set in multiple places, e.g., the two scenarios of manually, directly deleting a g.o. via "delete(go)" versus the delete initiating from a parent being closed or deleted.

Dan Sebald <sebald>
Thu 14 Dec 2017 11:22:25 PM UTC, comment #5: 

Mine also hangs when run alone, but if I run it under gdb
(./run-octave -g) it gets killed:




octave:octave:1> fcn = @(h) close (ancestor (h, "figure"));
octave:2> hl = line ("deletefcn", fcn);
Detaching after fork from child process 6449.
Detaching after fork from child process 6450.
octave:3> delete (hl)

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7651cb8 in graphics_object::finalize (this=0xcbf8b8) at libinterp/corefcn/graphics.h:3073


This is a minimal version (no qt no fltk) compiled with debug
option.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 14 Dec 2017 11:18:23 PM UTC, comment #4: 

When running with a version with ASAN enabled, I get


==26167==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060011d2868 at pc 0x7f6e29d5ccf3 bp 0x7fff78bdec60 sp 0x7fff78bdec50
READ of size 8 at 0x6060011d2868 thread T0
    #0 0x7f6e29d5ccf2 in std::__shared_ptr<base_graphics_object, (__gnu_cxx::_Lock_policy)2>::operator->() const /usr/include/c++/5/bits/shared_ptr_base.h:1055
    #1 0x7f6e29f04bad in graphics_object::finalize() libinterp/corefcn/graphics.h:3073
    #2 0x7f6e29d848d3 in gh_manager::do_free(octave_handle const&) libinterp/corefcn/graphics.cc:2667
    #3 0x7f6e29f3fca8 in gh_manager::free(octave_handle const&) libinterp/corefcn/graphics.h:12922
    #4 0x7f6e29d85bcc in delete_graphics_object libinterp/corefcn/graphics.cc:2812
    #5 0x7f6e29d85d5b in delete_graphics_object libinterp/corefcn/graphics.cc:2826
    #6 0x7f6e29d85e7e in delete_graphics_objects libinterp/corefcn/graphics.cc:2841
    #7 0x7f6e29eea704 in F__go_delete__(octave_value_list const&, int) libinterp/corefcn/graphics.cc:11449


Just glancing through the code, there are several comments about avoiding problems just like this one where the child or parent graphics object has already been destroyed.  Obviously, it isn't perfect, but extreme care was taken in coding this up.


Rik <rik5>
Group administrator
Thu 14 Dec 2017 10:52:51 PM UTC, comment #3: 

I don't get a segfault, but Octave does hang and uses 100% of the CPU until killed.

The deletefcn() is called prior to actually destroying the object so that the callback can inspect any properties of the object it needs before allowing the destruction to occur.

Calling close, or delete, on the parent figure will cause the graphics system to recursively attempt to delete each child handle of the figure which will eventually call delete() again on this handle.  It make be that, for me, the object is locked because the deletefcn() is executing and thus the system hangs.

Rik <rik5>
Group administrator
Thu 14 Dec 2017 10:29:27 PM UTC, comment #2: 

Backtrace that i got (no asan):


#0  0x00007ffff7651cb8 in graphics_object::finalize (this=0xcbf8b8) at libinterp/corefcn/graphics.h:3073
#1  0x00007ffff752b52c in gh_manager::do_free (this=0xad6b50, h=...) at ../libinterp/corefcn/graphics.cc:2667
#2  0x00007ffff767829b in gh_manager::free (h=...) at libinterp/corefcn/graphics.h:12922
#3  0x00007ffff752bdee in delete_graphics_object (h=...) at ../libinterp/corefcn/graphics.cc:2812
#4  0x00007ffff752be9a in delete_graphics_object (val=-6.33522275586835) at ../libinterp/corefcn/graphics.cc:2826
#5  0x00007ffff752bf07 in delete_graphics_objects (vals=...) at ../libinterp/corefcn/graphics.cc:2841
#6  0x00007ffff76421fe in F__go_delete__ (args=...) at ../libinterp/corefcn/graphics.cc:11449
#7  0x00007ffff71cc4ac in octave_builtin::call (this=0x6d7520, tw=..., nargout=0, args=...) at ../libinterp/octave-value/ov-builtin.cc:65
#8  0x00007ffff73871c5 in octave::tree_evaluator::visit_index_expression (this=0x669908, idx_expr=...) at ../libinterp/parse-tree/pt-eval.cc:1332
#9  0x00007ffff73a6b2e in octave::tree_index_expression::accept (this=0x11027c0, tw=...) at ../libinterp/parse-tree/pt-idx.h:101
#10 0x00007ffff721e520 in octave::tree_evaluator::evaluate (this=0x669908, expr=0x11027c0, nargout=0) at ../libinterp/parse-tree/pt-eval.h:276
#11 0x00007ffff738ba1d in octave::tree_evaluator::visit_statement (this=0x669908, stmt=...) at ../libinterp/parse-tree/pt-eval.cc:2295
#12 0x00007ffff73afaa6 in octave::tree_statement::accept (this=0x127c8c0, tw=...) at ../libinterp/parse-tree/pt-stmt.h:111
#13 0x00007ffff738bbc4 in octave::tree_evaluator::visit_statement_list (this=0x669908, lst=...) at ../libinterp/parse-tree/pt-eval.cc:2337
#14 0x00007ffff721e95a in octave::tree_statement_list::accept (this=0x11b46d0, tw=...) at ../libinterp/parse-tree/pt-stmt.h:186
#15 0x00007ffff73868d5 in octave::tree_evaluator::visit_if_command_list (this=0x669908, lst=...) at ../libinterp/parse-tree/pt-eval.cc:1141
#16 0x00007ffff737e028 in octave::tree_if_command_list::accept (this=0x8119e0, tw=...) at ../libinterp/parse-tree/pt-select.h:115
#17 0x00007ffff7386734 in octave::tree_evaluator::visit_if_command (this=0x669908, cmd=...) at ../libinterp/parse-tree/pt-eval.cc:1120
#18 0x00007ffff73ae7dc in octave::tree_if_command::accept (this=0xda87b0, tw=...) at ../libinterp/parse-tree/pt-select.h:148
#19 0x00007ffff738b8b2 in octave::tree_evaluator::visit_statement (this=0x669908, stmt=...) at ../libinterp/parse-tree/pt-eval.cc:2258
#20 0x00007ffff73afaa6 in octave::tree_statement::accept (this=0xe10370, tw=...) at ../libinterp/parse-tree/pt-stmt.h:111
#21 0x00007ffff738bbc4 in octave::tree_evaluator::visit_statement_list (this=0x669908, lst=...) at ../libinterp/parse-tree/pt-eval.cc:2337
#22 0x00007ffff721e95a in octave::tree_statement_list::accept (this=0x1227730, tw=...) at ../libinterp/parse-tree/pt-stmt.h:186
#23 0x00007ffff72b5256 in octave_user_function::call (this=0xe86d00, tw=..., nargout=0, _args=...) at ../libinterp/octave-value/ov-usr-fcn.cc:653
#24 0x00007ffff73871c5 in octave::tree_evaluator::visit_index_expression (this=0x669908, idx_expr=...) at ../libinterp/parse-tree/pt-eval.cc:1332
#25 0x00007ffff73a6b2e in octave::tree_index_expression::accept (this=0x111c770, tw=...) at ../libinterp/parse-tree/pt-idx.h:101
#26 0x00007ffff721e520 in octave::tree_evaluator::evaluate (this=0x669908, expr=0x111c770, nargout=0) at ../libinterp/parse-tree/pt-eval.h:276
#27 0x00007ffff738ba1d in octave::tree_evaluator::visit_statement (this=0x669908, stmt=...) at ../libinterp/parse-tree/pt-eval.cc:2295
#28 0x00007ffff73afaa6 in octave::tree_statement::accept (this=0x124cc80, tw=...) at ../libinterp/parse-tree/pt-stmt.h:111
#29 0x00007ffff738bbc4 in octave::tree_evaluator::visit_statement_list (this=0x669908, lst=...) at ../libinterp/parse-tree/pt-eval.cc:2337
#30 0x00007ffff721e95a in octave::tree_statement_list::accept (this=0xe09240, tw=...) at ../libinterp/parse-tree/pt-stmt.h:186
#31 0x00007ffff76ae8b0 in octave::interpreter::main_loop (this=0x669570) at ../libinterp/corefcn/interpreter.cc:974
#32 0x00007ffff76ad761 in octave::interpreter::execute (this=0x669570) at ../libinterp/corefcn/interpreter.cc:695
#33 0x00007ffff6e68ca1 in octave::cli_application::execute (this=0x7fffffffc3e0) at ../libinterp/octave.cc:384
#34 0x000000000040184a in main (argc=8, argv=0x7fffffffc708) at ../src/main-cli.cc:90


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 14 Dec 2017 07:47:10 PM UTC, comment #1: 

Confirmed on Win 10 with dev Octave

OS => Any
Release => dev

Philip Nienhuis <philipnienhuis>
Group Member
Thu 14 Dec 2017 07:14:05 PM UTC, original submission:  

The following crashes both stable and dev Octave with any toolkit:


fcn = @(h) close (ancestor (h, "figure"));
hl = line ("deletefcn", fcn);
delete (hl)


For some reason I can't get a usable backtrace, it looks like this (address sanitizer enabled):


==17665== ERROR: AddressSanitizer: heap-use-after-free on address 0x600c00c3b808 at pc 0x7ffff47a46b7 bp 0x7fffd3359c90 sp 0x7fffd3359c88
READ of size 8 at 0x600c00c3b808 thread T2 (QThread)
    #0 0x7ffff47a46b6 (/home/pantxo/Documents/On/octavebuild/libgui/.libs/liboctgui.so.2.0.0+0x34e6b6)
    #1 0x7ffff3968fa1 (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x1212fa1)
    #2 0x7ffff3816109 (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x10c0109)
    #3 0x7ffff3992958 (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x123c958)
    #4 0x7ffff3816e8d (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x10c0e8d)
    #5 0x7ffff3816f62 (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x10c0f62)
    #6 0x7ffff3817024 (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x10c1024)
    #7 0x7ffff3953075 (/home/pantxo/Documents/On/octavebuild/libinterp/.libs/liboctinterp.so.4.0.0+0x11fd075)


I tried to play with the "interruptible" property of the line object but it does not seam to do the job.

Pantxo Diribarne <pantxo>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by pantxo (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-17 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2017-12-15 rik5 StatusConfirmed Patch Reviewed
    2017-12-15 sebald Attached File- Added octave-deletefcn_close-djs2017dec14.patch, #42653
    2017-12-14 philipnienhuis StatusNone Confirmed
        Release4.2.1 dev
        Operating SystemGNU/Linux Any

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code