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

 
 

bug #23364: premature destruction of ref-counted object

Submitter:  Sandro Santilli <strk>
Submitted:  Mon 26 May 2008 04:33:42 PM UTC
   
 
Category:  core Severity:  6 - Security
Release:  None Status:  Fixed
Privacy:  Public Assigned to:  strk
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

  Spam posted by 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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
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 bwy (Posted a comment)
  • -email is unavailable- added by strk
  • -email is unavailable- added by zoulunkai (Posted a comment)
  • -email is unavailable- added by strk
  • -email is unavailable- added by strk
  • -email is unavailable- added by strk (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
    2008-07-22 strk StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2008-05-30 strk StatusIn Progress Ready For Test
        Carbon-Copy- Added nelsonrn
    2008-05-29 strk Carbon-Copy- Added zoulunkai
    2008-05-28 strk StatusNone In Progress
    2008-05-28 strk Assigned toNone strk
        Carbon-Copy- Added udog

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code