bugGnash - The GNU Flash player - Bugs: bug #23364, premature destruction of...

 
 

bug #23364: premature destruction of ref-counted object

Submitted by:  Sandro Santilli <strk>
Submitted on:  Mon 26 May 2008 04:33:42 PM UTC  
 
Category: coreSeverity: 6 - Security
Release: NoneStatus: Fixed
Privacy: PublicAssigned to: Sandro Santilli <strk>
Open/Closed: Closed

Add a New Comment (Rich MarkupRich 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)

 Spam posted by an anonymous
Fri 13 Jun 2008 06:06:28 PM UTC, comment #15:

I guess it is still worth backporting to the 083 branch
we might get out somethign from it in form of 0.8.3.1, for
major bugs..

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 13 Jun 2008 04:32:26 PM UTC, comment #14:

I guess it's too late for this, unless anyone plans on maintaining 0.8.3

Benjamin Wolsey <bwy>
Project Member
Sat 07 Jun 2008 05:39:51 PM UTC, comment #13:

IIRC I already backported to branch.
Problem was with IMPORT tag.
The gprocessor fix might be worth backporting too ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Sat 07 Jun 2008 04:43:15 PM UTC, comment #12:

I can confirm the fix, I think, as I had to 'port' it to gprocessor to get the testsuite to stop segfaulting with movielibrarylimit of 1.

Benjamin Wolsey <bwy>
Project Member
Fri 30 May 2008 08:23:55 PM UTC, comment #11:

Committed a patch adding to movies a set of references to movies
it imports symbols from.
I think this is worth backporting to branch, what do you think ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 30 May 2008 04:25:58 PM UTC, comment #10:

I also found the reason why a limit of 0 in the MovieLibrary
doesn't trigger the bug:

If limit is 0, MovieLibrary would just discard the passed
definition.

MovieLibrary::add() doesn't take an intrusive_ptr,
so you don't need to construct one.

The IMPORT tag loader, never constructs an intrusive_ptr.
Thust, when MovieLibrary limit is zero, the definition
just leaks forever :)

==18912== 50,170 (692 direct, 49,478 indirect) bytes in 1 blocks are definitely lost in loss record 52 of 70
==18912== at 0x4023294: operator new(unsigned) (vg_replace_malloc.c:224)
==18912== by 0x42B70DD: gnash::create_movie(std::auto_ptr<tu_file>, std::string const&, bool) (impl.cpp:471)
==18912== by 0x42B757C: gnash::create_movie(gnash::URL const&, char const, bool, std::string const) (impl.cpp:543)
==18912== by 0x42B7BFB: gnash::create_library_movie(gnash::URL const&, char const, bool, std::string const) (impl.cpp:788)
==18912== by 0x420D0F7: gnash::SWF::tag_loaders::import_loader(gnash::stream, gnash::SWF::tag_type, gnash::movie_definition) (tag_loaders.cpp:1089)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 30 May 2008 04:12:24 PM UTC, comment #9:

So, it seems movie_instance already keeps it's def alive (stores by intrusive_ptr).
Then it looks like this bug is really only related
to IMPORT for now.. I'll try to fix by adding
a list of import sources in movie_def_impl.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 30 May 2008 06:26:38 AM UTC, comment #8:

I suggest the discussion about memory management for definitions
be kept on gnash-dev mailing list, and keeping this item
only for the specific bug exposed by setting libraryMovieLimit
to 1 and running the MovieClipLoader-v7.swf testcase.

I belive in that case the problem is:

- first movie gets into the MovieLibrary (1 item)
- during parsing of first movie, IMPORT tag is found,
triggering load of the imported movie (Dejagnu.swf).
loaded movie is pushed to the MovieLibrary, which
drops the first movie (to maintain the limit to 1)
- sometime before the first movie is registered into
the movie_root (which would keep it alive) its refcount
goes down to zero.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 30 May 2008 06:22:25 AM UTC, comment #7:

> ActionScripts could never access any DEFINITIONS directly.


Not directly, but indirectly yes, for instance symbols
in the symbols library can be accessed using attachMovie()
and registerClass(). Also, the security engine should be
able to tell which action block came from which movie.
Finally, _url, bytes loaded/total from an instance would access
the definition.

> All DEFINITONS are reference counted by their INSTANCES, but
> not any other defintions. If there's no live instances for a
> certain definiton, then definiton would be destroyed.


Can't work, as definitions are accessed trough characted id
by a lookup in the Dictionary, this by PlaceObject (still
found in definition, btw)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 30 May 2008 01:32:36 AM UTC, comment #6:

I understand the GC mechanism need to be carefully maintained for all ActionScriptable INSTANCES, but not sure about what's the difficulty for DEFINITIONS.

Basically, each swf defined resource should be owned by a movie_definition class. And movie_definitions themself are owned by the MovieLibrary class. ActionScripts could never access any DEFINITIONS directly.

Now, what about this model?

All DEFINITONS are reference counted by their INSTANCES, but not any other defintions. If there's no live instances for a certain definiton, then definiton would be destroyed.

eg1. the main movie definiton is always owned by _root instances, so would never be destroyed.

eg2. mc.loadMovie("external_resource.swf");
Now, mc is the instance which references the definiton of external_resource.swf. Then definiton of external_resource.swf will be destroyed if(1) mc is destroyed or (2)mc doesn't reference external_resource.swf any more(by unloadMovie()).

For the implementation: we might use reference counted pointer to definition in every character instance, and raw pointers inside and between defintions themself.

There's no reference cycles or premature deletions to me. Comments?

Zou Lunkai <zoulunkai>
Thu 29 May 2008 03:30:56 PM UTC, comment #5:

zou: remember we discussed about whether or not to have the GC manage definitions too ?
For this case it seems a solution to me.

Another one would likely be carefully reviewing reasons
to keep those references to movie_def, but I'm sure there
are, even more then we use to have now (security paths following)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Thu 29 May 2008 03:27:17 PM UTC, comment #4:

So, the problem is sprite_movie_def
NOT keeping the m_movie_def member (the movie_definition
containing the sprite definition) by intrusive_ptr.

It's not explicitly written, but I guess that was
to avoid circular intrusive_ptr refs, which would
result in never deleting the partecipating objects.

Still, the movie definition dies (likely Dejagnu.swf)
while the sprite_definition remains (the dejagnu sprite)
and tries to access deleted memory for a lot of things
(bytes loaded/total, version, url).

No quick and easy solution I can see here (beside letting
it leak)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Wed 28 May 2008 11:52:07 PM UTC, comment #3:

In this case, of the 2 movies in library (MovieLoader-v7.swf and Dejagnu.swf), Dejagnu.swf is dying prematurely.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Wed 28 May 2008 11:49:51 PM UTC, comment #2:

The still-needed movie_def is removed by limit_size

==1364== Invalid read of size 4
==1364== at 0x4426C2C: gnash::sprite_definition::get_version() const (sprite_definition.h:127)
==1364== by 0x41C6BE4: gnash::sprite_instance::getAsRoot() const (sprite_instance.h:138)
==1364== by 0x41E2628: gnash::sprite_instance::get_member(unsigned, gnash::as_value*, unsigned) (sprite_instance.cpp:2463)
==1364== by 0x41124CE: gnash::as_environment::get_variable_raw(std::string const&, std::__debug::vector<boost::intrusive_ptr<gnash::as_object>, std::allocator<boost::intrusive_ptr<gnash::as_object> > > const&, gnash::as_object**) const (as_environment.cpp:185)
==1364== by 0x41128E2: gnash::as_environment::get_variable(std::string const&, std::__debug::vector<boost::intrusive_ptr<gnash::as_object>, std::allocator<boost::intrusive_ptr<gnash::as_object> > > const&, gnash::as_object**) const (as_environment.cpp:105)
==1364== by 0x4483F91: gnash::ActionExec::getVariable(std::string const&) (ActionExec.cpp:556)
==1364== by 0x444B4D2: gnash::SWF::SWFHandlers::ActionGetVariable(gnash::ActionExec&) (ASHandlers.cpp:976)
==1364== by 0x442E6D1: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:166)
==1364== by 0x443EF49: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:449)
==1364== by 0x44857D8: gnash::ActionExec::operator()() (ActionExec.cpp:339)
==1364== by 0x4224872: gnash::swf_function::operator()(gnash::fn_call const&) (swf_function.cpp:303)
==1364== by 0x449687C: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, int, int, gnash::as_object*) (action.cpp:79)
==1364== Address 0x59b2610 is 0 bytes inside a block of size 692 free'd
==1364== at 0x40222EC: operator delete(void*) (vg_replace_malloc.c:342)
==1364== by 0x43F1692: gnash::movie_def_impl::~movie_def_impl() (movie_def_impl.cpp:246)
==1364== by 0x42C0E6C: std::__debug::map<std::string, gnash::MovieLibrary::item, std::less<std::string>, std::allocator<std::pair<std::string const, gnash::MovieLibrary::item> > >::erase(__gnu_debug::_Safe_iterator<std::_Rb_tree_iterator<std::pair<std::string const, gnash::MovieLibrary::item> >, std::__debug::map<std::string, gnash::MovieLibrary::item, std::less<std::string>, std::allocator<std::pair<std::string const, gnash::MovieLibrary::item> > > >) (ref_counted.h:87)
==1364== by 0x42C3862: gnash::MovieLibrary::limit_size(unsigned) (impl.cpp:715)

Sandro Santilli <strk>
Project MemberIn charge of this item.
Tue 27 May 2008 03:35:19 PM UTC, comment #1:

We shouldn't have this bug laying around.
Maybe a good short-term compromise is limiting
the "movieLibrary limit" to at least 2...

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 26 May 2008 04:33:42 PM UTC, original submission:

actionscript.all/MovieClipLoader-v7.swf segfaults for me
if I 'set movieLibraryLimit 1'.
We don't force that in testsuite/gnashrc currently, maybe
we should to catch more of these...

The problem lies somewhere in the core, where we just
assume that the MovieLibrary will keep things alive
for us. The same create_library_movie stores the newly
allocated thing in an intrusive_ptr but later returns
by raw pointer, thus if nobody else keeps a ref, the
returned pointer will point to deleted memory....

Sandro Santilli <strk>
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 bwy (Posted a comment)
  • -unavailable- added by strk
  • -unavailable- added by zoulunkai (Posted a comment)
  • -unavailable- added by strk
  • -unavailable- added by strk
  • -unavailable- added by strk (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
    Tue 22 Jul 2008 10:43:01 AM UTCstrkStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Fri 30 May 2008 08:23:55 PM UTCstrkStatusIn Progress=>Ready For Test
      Carbon-Copy-=>Added nelsonrn
    Thu 29 May 2008 03:30:56 PM UTCstrkCarbon-Copy-=>Added zoulunkai
    Wed 28 May 2008 11:59:37 PM UTCstrkStatusNone=>In Progress
    Wed 28 May 2008 11:49:51 PM UTCstrkAssigned toNone=>strk
      Carbon-Copy-=>Added udog

    Back to the top


    Powered by Savane 3.1-cleanup