bugGnash - The GNU Flash player - Bugs: bug #39404, invalid read from...

 
 

bug #39404: invalid read from XMLNode_as::clearChildren()

Submitted by:  Bastiaan Jacques <bjacques>
Submitted on:  Wed 03 Jul 2013 09:39:29 AM UTC  
 
Category: ActionScriptSeverity: 6 - Security
Release: masterStatus: Fixed
Privacy: PublicAssigned to: Bastiaan Jacques <bjacques>
Open/Closed: Closed

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)

Fri 01 Nov 2013 09:11:38 PM UTC, comment #14:

What if we just make the destructor do nothing, rather than trying to clear children ?

Sandro Santilli <strk>
Project Member
Thu 31 Oct 2013 09:18:00 PM UTC, comment #13:

Another related issue is bug #40440
all the worm off the can...

Sandro Santilli <strk>
Project Member
Thu 31 Oct 2013 08:47:08 PM UTC, comment #12:

See bug #40439 for a possible regression introduced by the fix for this. Luckly we have a testcase for this bug, so it'll be easier to find a solution for the new bug (which will need a testcase on itself)

Sandro Santilli <strk>
Project Member
Thu 04 Jul 2013 02:18:02 PM UTC, comment #11:

Thanks!

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Thu 04 Jul 2013 01:23:26 PM UTC, comment #10:

Tested with
http://git.savannah.gnu.org/gitweb/?p=gnash.git;a=commitdiff;h=fb23f4d

Sandro Santilli <strk>
Project Member
Thu 04 Jul 2013 01:01:14 PM UTC, comment #9:

Here's a test that makes valgrind capable of finding the problem:

var x = new XML('<t></t>');
x.appendChild(new XML('<t></t>'));
var x2 = new XML('<t></t>');
x2.appendChild(x);
// many allocations force GC run
for (var i=0; i<10000; ++i) x = {};

Build in version 6, possibly uncompressed.
Run with gprocessor. Valgrind reports:

==9087== Invalid read of size 8
==9087== at 0x51EDA1C: gnash::XMLNode_as::clearChildren() (XMLNode_as.cpp:384)
==9087== by 0x51EDA95: gnash::XMLNode_as::~XMLNode_as() (XMLNode_as.cpp:113)
==9087== by 0x51EBFB0: gnash::XML_as::~XML_as() (XML_as.h:77)
==9087== by 0x50A6491: gnash::as_object::~as_object() (checked_delete.hpp:34)
==9087== by 0x564DF9E: gnash::GC::~GC() (GC.cpp:62)
==9087== by 0x50E5B41: gnash::movie_root::~movie_root() (movie_root.cpp:190)
==9087== by 0x111CCE: play_movie(std::string const&, gnash::RunResources const&) (processor.cpp:602)
==9087== by 0x110AFB: main (processor.cpp:368)
==9087== Address 0x15f8c620 is 32 bytes inside a block of size 136 free'd
==9087== at 0x4C2A4BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9087== by 0x50A6491: gnash::as_object::~as_object() (checked_delete.hpp:34)
==9087== by 0x564DF9E: gnash::GC::~GC() (GC.cpp:62)
==9087== by 0x50E5B41: gnash::movie_root::~movie_root() (movie_root.cpp:190)
==9087== by 0x111CCE: play_movie(std::string const&, gnash::RunResources const&) (processor.cpp:602)
==9087== by 0x110AFB: main (processor.cpp:368)

Sandro Santilli <strk>
Project Member
Thu 04 Jul 2013 11:19:30 AM UTC, comment #8:

Yes, as explained on IRC if the GC cleans the parent before the child, the child will find it's _parent member to be NULL, as set by the parent's destructor (which won't access the child if the child was deleted before as it will unlink self from parent).

A testcase would need to run for multiple frames as GC only runs on frame boundary and only if enough GCResources were allocated since last run. So it'd take some onEnterFrame and big builds of XML, then deletion and more builds. Could be a fun task.

Maybe gg0 wants to try this one :)

Sandro Santilli <strk>
Project Member
Wed 03 Jul 2013 06:56:01 PM UTC, comment #7:

I think Sandro now understands the patch.

As for a testcase, so far I have unfortunately not figured how to reduce a problematic SWF into a testcase. The simplest movie I've found so far that exhibits the problem is this one:

http://www.myclubdesign.com/WebRoot/ce_fr3/Shops/270286/MediaGallery/animations_flash/logopartenaires.swf

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Wed 03 Jul 2013 03:24:07 PM UTC, comment #6:

Bastiaan please drop a reference to a commit when closing tickets.

It looks like your change just shakes the perspective, not really fixing the problem:

http://git.savannah.gnu.org/gitweb/?p=gnash.git;a=commitdiff;h=c5f7578

The problem as I see it is that objects are assuming other objects they don't own are alive. What if the GC cleans the parent before the child ? We'd be back to memory error.

Could you add a simplified testcase to exploit the problem ?

Sandro Santilli <strk>
Project Member
Wed 03 Jul 2013 01:43:36 PM UTC, comment #5:

GcResources are owned by the GC, so if your theory is correct the bug is in the node checking its children (ie: assuming they are alive) at destruction time.

Could all child be GcResources ?
Or could being or not a GcResource be registered somewhere else ?

Sandro Santilli <strk>
Project Member
Wed 03 Jul 2013 12:55:08 PM UTC, comment #4:

So here's my theory.

Supposing a simple parsed XML tree:

XMLobj -> child -> grandchild

Where XMLobj is the root node, which is kept alive by AS. Let's assume XMLobj clears its tree by parsing a new XML document. Because XMLobj owns child, it can simply forget about child, and child will automatically be garbage-collected because child's setReachable will no longer be called. In turn, grandchild will also be garbage collected, because it is owned by child.

The complication arises when a node clears its children: because not all children are GcResources, it checks whether a child is a GcResource by testing if it has an associated as_object, and deletes if the child doesn't.

This works fine, but this presumes that child and grandchild are garbage-collected in order of precedence (i.e., parent first). As it turns out, the garbage collector is just as, or perhaps more, likely to destroy grandchild before child. And in that case, when child is deleted after grandchild, child attempts to probe grandchild, which is already deleted.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Wed 03 Jul 2013 10:01:30 AM UTC, comment #3:

Something is in reach of the XMLNode and not marking it...
What ? Hard to tell. Time to find the simplest case and simplify it further till you get to figure out.

Sandro Santilli <strk>
Project Member
Wed 03 Jul 2013 09:59:17 AM UTC, comment #2:

There is code serving that purpose in XMLNode::setReachable(), and it is evidently not being called, but I'm not sure why.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Wed 03 Jul 2013 09:56:39 AM UTC, comment #1:

XMLNode not marking childs as reachable ?

Sandro Santilli <strk>
Project Member
Wed 03 Jul 2013 09:39:29 AM UTC, original submission:

Valgrind shows the following error for quite a few movies, Youtube among which.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by strk (Posted a comment)
  • -unavailable- added by bjacques (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 logged-in users can vote.

     

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

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 04 Jul 2013 01:23:26 PM UTCstrkStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 03 Jul 2013 03:24:07 PM UTCstrkStatusFixed=>Ready For Test
      Open/ClosedClosed=>Open
    Wed 03 Jul 2013 02:13:47 PM UTCbjacquesStatusNone=>Fixed
      Assigned toNone=>bjacques
      Open/ClosedOpen=>Closed
    Wed 03 Jul 2013 12:55:08 PM UTCbjacquesSeverity4 - Important=>6 - Security

    Back to the top


    Powered by Savane 3.1-cleanup1