bugGNU Octave - Bugs: bug #51088, double free at Octave exit

 
 

bug #51088: double free at Octave exit

Submitted by:  Olaf Till <i7tiol>
Submitted on:  Mon 22 May 2017 10:00:38 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: None
Originator Name: Olaf TillOpen/Closed: Closed
Release: 4.2.1Operating 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)

Wed 31 May 2017 05:04:30 AM UTC, comment #8:

Tested patches in both branches with the demonstration code, no segfault.

Olaf Till <i7tiol>
Project Member
Tue 30 May 2017 04:50:53 PM UTC, comment #7:

I think we can do the same thing in erase_scope: save pointer to table, remove element from the map, delete table. I pushed the following changesets to do that on stable and default:

http://hg.savannah.gnu.org/hgweb/octave/rev/25817ecc6123
http://hg.savannah.gnu.org/hgweb/octave/rev/61971e3f1946

Thanks.

John W. Eaton <jwe>
Project Administrator
Sun 28 May 2017 06:38:49 PM UTC, comment #6:

Yes, absolutely. Take this instead of my patch. But have you considered the part for symbtab.h (symbol_table::erase_scope), should the symbol_table pointer not be zeroed there in all_instances before deleting (a pointer copy)? (Although the demonstrated misbehaviour didn't depend on it.)

Olaf Till <i7tiol>
Project Member
Sun 28 May 2017 05:07:33 PM UTC, comment #5:

Thanks for the report and patch. I think the attached version is a bit simpler and clearer.

(file #40804)

John W. Eaton <jwe>
Project Administrator
Wed 24 May 2017 10:05:42 AM UTC, comment #4:

The second half of the changeset is not necessary to fix the bug as it was currently triggered. But I thought a similar bug could occur if this change isn't made, too.

To explain the second half:

To your question: The destructor of the symbol_table (being empty, but automatically calling the destructors of the contained objects), is called by deleting with the copy 'inst' of 'p->second'. The '...->second' entries in 'all_instances' are zeroed before triggering the destructor call to avoid that the side effects of the destructor may try to trigger a destructor of a specific symbol table a second time (over 'all_instances').

This deleting with a pointer copy, after zeroing the original pointer, is the same as was done already before my patch in symtab.cc, symbol_table::cleanup(), to avoid that side effects call destructors of already destroyed symbol_tables. Here, in symbtab.h, symbol_table::erase_scope(), my change is only a precaution against the possibility that the side effects could try to destroy (over 'all_instances') the same symbol table as we currently already are destroying. Or can someone exclude that this happens? I'd rather be at the safe side.

Olaf Till <i7tiol>
Project Member
Tue 23 May 2017 11:35:08 PM UTC, comment #3:

The changeset works. However, is the second half of the cset necessary?

I removed this part of the patch and it still continued to work. Also, it seems like we shouldn't be zeroing the pointer of the symbol_table since we want the symbol table destructor to be called if it exists. Thoughts on that?

Rik <rik5>
Project Administrator
Tue 23 May 2017 08:43:54 PM UTC, comment #2:

Confirmed. Even with the latest development branch code I get a segfault with the test code

Rik <rik5>
Project Administrator
Mon 22 May 2017 10:06:48 PM UTC, comment #1:

I have erroneously attached two versions of the changeset. Please take the later one.

Instead I had not attached the example code, which I do now.

(file #40762)

Olaf Till <i7tiol>
Project Member
Mon 22 May 2017 10:00:38 PM UTC, original submission:

(tested with 4.2.1, but probably affects all current versions)

Under certain conditions Octave segfaults at exit due to a double free. Example code, involving anonymous functions and persistent variables, is attached in 'test-double-free.tar.gz', trigger the bug (hopefully) by going into the directory and calling 'test_gph ()' .

Debugging showed the following reason:

symbol_table::cleanup(void) iterates over symbol_table::all_instances and deletes symbol tables. As a side effect of such a delete, symbol_table::erase_scope(scope_id) erased a scope/symbol_table-pointer pair from all_instances. This erasing messed up the outer iteration over all_instances, so that after the next incrementing of the iterator it points to an invalid pair, causing an attempt to delete with the same symbol_table pointer which was deleted with by symbol_table::erase_scope(scope_id) .

A changeset with an explanatory comment will be attached as soon as I have the bug number. The patch is against the head of the stable branch, but has been tested against the 4.2.1 release tarball.

Olaf Till <i7tiol>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #40804:  diffs.txt added by jwe (980B - text/plain)
file #40762:  test-double-free.tar.gz added by i7tiol (456B - application/x-tgz)
file #40761:  fix-double-delete.cset added by i7tiol (3KiB - application/octet-stream)
file #40759:  fix-double-delete.cset added by i7tiol (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jwe (Updated the item)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by i7tiol (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 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 31 May 2017 03:40:57 PM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Tue 30 May 2017 04:50:53 PM UTCjweStatusPatch Submitted=>Ready For Test
    Sun 28 May 2017 05:08:00 PM UTCjweStatusConfirmed=>Patch Submitted
    Sun 28 May 2017 05:07:33 PM UTCjweAttached File-=>Added diffs.txt, #40804
    Tue 23 May 2017 08:43:54 PM UTCrik5StatusNone=>Confirmed
    Mon 22 May 2017 10:06:48 PM UTCi7tiolAttached File-=>Added test-double-free.tar.gz, #40762
    Mon 22 May 2017 10:03:48 PM UTCi7tiolAttached File-=>Added fix-double-delete.cset, #40761
    Mon 22 May 2017 10:00:38 PM UTCi7tiolAttached File-=>Added fix-double-delete.cset, #40759

    Back to the top


    Powered by Savane 3.1-cleanup1