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

 
 

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

Submitter:  Bastiaan Jacques <bjacques>
Submitted:  Wed 03 Jul 2013 09:39:29 AM UTC
   
 
Category:  ActionScript Severity:  6 - Security
Release:  master Status:  Fixed
Privacy:  Public Assigned to:  bjacques
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group 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>
Group Member
Thu 04 Jul 2013 02:18:02 PM UTC, comment #11: 

Thanks!

Bastiaan Jacques <bjacques>
Group Member
Thu 04 Jul 2013 01:23:26 PM UTC, comment #10: 
Sandro Santilli <strk>
Group 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>
Group 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>
Group 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>
Group Member
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>
Group 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>
Group 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>
Group Member
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>
Group 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>
Group Member
Wed 03 Jul 2013 09:56:39 AM UTC, comment #1: 

XMLNode not marking childs as reachable ?

Sandro Santilli <strk>
Group 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.


==10762== Invalid read of size 8
==10762==    at 0x4DCD9AC: gnash::XMLNode_as::clearChildren() (XMLNode_as.cpp:384)
==10762==    by 0x4DCDA34: gnash::XMLNode_as::~XMLNode_as() (XMLNode_as.cpp:113)
==10762==    by 0x4DCDB30: gnash::XMLNode_as::~XMLNode_as() (XMLNode_as.cpp:114)
==10762==    by 0x4C9ED0B: gnash::as_object::~as_object() (checked_delete.hpp:34)
==10762==    by 0x4C9EE50: gnash::as_object::~as_object() (as_object.h:173)
==10762==    by 0x56FA108: gnash::GC::cleanUnreachable() (GC.cpp:84)
==10762==    by 0x56FA16C: gnash::GC::runCycle() (GC.cpp:125)
==10762==    by 0x4CF1947: gnash::movie_root::advance() (movie_root.cpp:932)
==10762==    by 0x14C166: gnash::Gui::advanceMovie(bool) (gui.cpp:953)
==10762==    by 0x1656ED: gnash::Gui::advance_movie(gnash::Gui*) (gui.h:271)
==10762==    by 0x6C13962: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
==10762==    by 0x6C12E05: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3600.3)
==10762==  Address 0x153f51d0 is 32 bytes inside a block of size 96 free'd
==10762==    at 0x4A078DE: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10762==    by 0x4C9ED0B: gnash::as_object::~as_object() (checked_delete.hpp:34)
==10762==    by 0x4C9EE50: gnash::as_object::~as_object() (as_object.h:173)
==10762==    by 0x56FA108: gnash::GC::cleanUnreachable() (GC.cpp:84)
==10762==    by 0x56FA16C: gnash::GC::runCycle() (GC.cpp:125)
==10762==    by 0x4CF1947: gnash::movie_root::advance() (movie_root.cpp:932)
==10762==    by 0x14C166: gnash::Gui::advanceMovie(bool) (gui.cpp:953)
==10762==    by 0x1656ED: gnash::Gui::advance_movie(gnash::Gui*) (gui.h:271)
==10762==    by 0x6C13962: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
==10762==    by 0x6C12E05: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3600.3)
==10762==    by 0x6C13157: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
==10762==    by 0x6C13559: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3600.3)


Bastiaan Jacques <bjacques>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-07-04 strk StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2013-07-03 strk StatusFixed Ready For Test
        Open/ClosedClosed Open
    2013-07-03 bjacques StatusNone Fixed
        Assigned toNone bjacques
        Open/ClosedOpen Closed
    2013-07-03 bjacques Severity4 - Important 6 - Security

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code