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

 
 

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

Submitted by:  Pantxo Diribarne <pantxo>
Submitted on:  Thu 14 Dec 2017 07:14:05 PM UTC  
 
Category: PlottingSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating 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 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>
Project 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>
Project 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:

And I see:

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:

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:

and here is the output:

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:

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

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

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:

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

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>
Project 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>
Project Administrator
Thu 14 Dec 2017 10:29:27 PM UTC, comment #2:

Backtrace that i got (no asan):

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>
Project Member
Thu 14 Dec 2017 07:14:05 PM UTC, original submission:

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

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

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

Pantxo Diribarne <pantxo>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by sebald (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by dasergatskov (Posted a comment)
  • -unavailable- added by pantxo (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 17 Dec 2017 02:43:33 AM UTCrik5StatusPatch Reviewed=>Fixed
      Open/ClosedOpen=>Closed
    Fri 15 Dec 2017 10:07:04 PM UTCrik5StatusConfirmed=>Patch Reviewed
    Fri 15 Dec 2017 04:46:26 AM UTCsebaldAttached File-=>Added octave-deletefcn_close-djs2017dec14.patch, #42653
    Thu 14 Dec 2017 07:47:10 PM UTCphilipnienhuisStatusNone=>Confirmed
      Release4.2.1=>dev
      Operating SystemGNU/Linux=>Any

    Back to the top


    Powered by Savane 3.1-cleanup1