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

 
 

bug #51088: double free at Octave exit

Submitter:  Olaf Till <i7tiol>
Submitted:  Mon 22 May 2017 10:00:38 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Olaf Till Open/Closed:  * Closed
Release:  * 4.2.1 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group 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>
Group 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>
Group 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>
Group Member
Tue 23 May 2017 11:35:08 PM UTC, comment #3: 

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


diff -r 22fd2ad85218 -r 784c3cb82f8a libinterp/corefcn/symtab.h
--- a/libinterp/corefcn/symtab.h        Thu May 18 08:09:08 2017 -0700
+++ b/libinterp/corefcn/symtab.h        Mon May 22 22:57:56 2017 +0200
@@ -1222,7 +1222,12 @@

     if (p != all_instances.end ())
       {
-        delete p->second;
+        // First zero the table entry to avoid possible duplicate delete.
+        symbol_table *inst = p->second;
+        p->second = 0;
+
+        // Now delete the scope.
+        delete inst;

         all_instances.erase (p);


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>
Group 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


test_gph
exit



Rik <rik5>
Group 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>
Group 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>
Group Member

 

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

Attach Files:
   
   
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
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by i7tiol (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-05-31 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-05-30 jwe StatusPatch Submitted Ready For Test
    2017-05-28 jwe StatusConfirmed Patch Submitted
    2017-05-28 jwe Attached File- Added diffs.txt, #40804
    2017-05-23 rik5 StatusNone Confirmed
    2017-05-22 i7tiol Attached File- Added test-double-free.tar.gz, #40762
    2017-05-22 i7tiol Attached File- Added fix-double-delete.cset, #40761
    2017-05-22 i7tiol Attached File- Added fix-double-delete.cset, #40759

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code