bugGnash - The GNU Flash player - Bugs: bug #42420, data races from...

 
 

bug #42420: data races from SWFMovieDefinition::read_all_swf()

Submitter:  Bastiaan Jacques <bjacques>
Submitted:  Sat 24 May 2014 11:06:25 PM UTC
   
 
Category:  core Severity:  4 - Important
Release:  master Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 04 Feb 2015 11:18:08 PM UTC, comment #10: 

Asan reports the following use-after-free after playing the attached PDF for a few seconds.


==24477==ERROR: AddressSanitizer: heap-use-after-free on address 0x619000009688 at pc 0x7fbac034c593 bp 0x7fffa868ec00 sp 0x7fffa868ebf8
READ of size 8 at 0x619000009688 thread T0
    #0 0x7fbac034c592 in boost::intrusive_ptr<gnash::SWF::ControlTag>::operator->() const /usr/include/boost/smart_ptr/intrusive_ptr.hpp:162:9
    #1 0x7fbac0343a42 in gnash::MovieClip::executeFrameTags(unsigned long, gnash::DisplayList&, int) /home/bastiaan/gnash/libcore/MovieClip.cpp:1059:17
    #2 0x7fbac03432b5 in gnash::MovieClip::advance() /home/bastiaan/gnash/libcore/MovieClip.cpp:940:17
    #3 0x7fbac02f0805 in gnash::SWFMovie::advance() /home/bastiaan/gnash/libcore/SWFMovie.cpp:82:5
    #4 0x7fbac02fabd7 in gnash::movie_root::advanceLiveChars() /home/bastiaan/gnash/libcore/movie_root.cpp:2061:9
    #5 0x7fbac02fa225 in gnash::movie_root::advanceMovie() /home/bastiaan/gnash/libcore/movie_root.cpp:968:5
    #6 0x7fbac02f9e9a in gnash::movie_root::advance() /home/bastiaan/gnash/libcore/movie_root.cpp:933:17
    #7 0x7fbac0faeeaf in gnash::Gui::advanceMovie(bool) /home/bastiaan/gnash/gui/gui.cpp:954:27
    #8 0x7fbac0fe4769 in gnash::NullGui::run() /home/bastiaan/gnash/gui/NullGui.cpp:44:5
    #9 0x7fbac0fcad42 in gnash::Player::run(int, char**, std::string const&, std::string const&) /home/bastiaan/gnash/gui/Player.cpp:664:5
    #10 0x7fbac0f5d14c in playFile(gnash::Player&, int, char**, std::string const&) /home/bastiaan/gnash/gui/gnash.cpp:92:5
    #11 0x7fbac0f647c1 in void std::_Bind<void (*(std::reference_wrapper<gnash::Player>, int, char**, std::_Placeholder<1>))(gnash::Player&, int, char**, std::string const&)>::__call<void, std::string&, 0ul, 1ul, 2ul, 3ul>(std::tuple<std::string&>&&, std::_Index_tuple<0ul, 1ul, 2ul, 3ul>) /bin/../lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/functional:1263:11
    #12 0x7fbac0f64176 in void std::_Bind<void (*(std::reference_wrapper<gnash::Player>, int, char**, std::_Placeholder<1>))(gnash::Player&, int, char**, std::string const&)>::operator()<std::string&, void>(std::string&) /bin/../lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/functional:1321:11
    #13 0x7fbac0f63c12 in std::_Bind<void (*(std::reference_wrapper<gnash::Player>, int, char**, std::_Placeholder<1>))(gnash::Player&, int, char**, std::string const&)> std::for_each<__gnu_cxx::__normal_iterator<std::string*, std::vector<std::string, std::allocator<std::string> > >, std::_Bind<void (*(std::reference_wrapper<gnash::Player>, int, char**, std::_Placeholder<1>))(gnash::Player&, int, char**, std::string const&)> >(__gnu_cxx::__normal_iterator<std::string*, std::vector<std::string, std::allocator<std::string> > >, __gnu_cxx::__normal_iterator<std::string*, std::vector<std::string, std::allocator<std::string> > >, std::_Bind<void (*(std::reference_wrapper<gnash::Player>, int, char**, std::_Placeholder<1>))(gnash::Player&, int, char**, std::string const&)>) /bin/../lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/stl_algo.h:3755:2
    #14 0x7fbac0f5d885 in main /home/bastiaan/gnash/gui/gnash.cpp:175:9
    #15 0x7fbabca43fdf in __libc_start_main (/lib64/libc.so.6+0x3629e1ffdf)
    #16 0x7fbac0f5cf7c in _start (/home/bastiaan/obj-gnash-clang-sanitize/gui/.libs/lt-gtk-gnash+0xf2f7c)

0x619000009688 is located 8 bytes inside of 1024-byte region [0x619000009680,0x619000009a80)
freed by thread T1 here:
    #0 0x7fbac0ed85ab in operator delete(void*) (/home/bastiaan/obj-gnash-clang-sanitize/gui/.libs/lt-gtk-gnash+0x6e5ab)
    #1 0x7fbac05a06a2 in void std::vector<boost::intrusive_ptr<gnash::SWF::ControlTag>, std::allocator<boost::intrusive_ptr<gnash::SWF::ControlTag> > >::_M_emplace_back_aux<boost::intrusive_ptr<gnash::SWF::ControlTag> const&>(boost::intrusive_ptr<gnash::SWF::ControlTag> const&) /bin/../lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/vector.tcc:438:2
    #2 0x7fbac058ca71 in gnash::SWFMovieDefinition::addControlTag(boost::intrusive_ptr<gnash::SWF::ControlTag>) /home/bastiaan/gnash/libcore/parser/SWFMovieDefinition.h:274:9
    #3 0x7fbac0586387 in gnash::SWFMovieDefinition::addDisplayObject(unsigned short, gnash::SWF::DefinitionTag*) /home/bastiaan/gnash/libcore/parser/SWFMovieDefinition.cpp:163:5
    #4 0x7fbac05802fd in gnash::SWFParser::read(long) /home/bastiaan/gnash/libcore/parser/SWFParser.cpp:96:17
    #5 0x7fbac0585738 in gnash::SWFMovieDefinition::read_all_swf() /home/bastiaan/gnash/libcore/parser/SWFMovieDefinition.cpp:467:18
    #6 0x7fbabd0b2d9f (/lib64/libstdc++.so.6+0x3b79ebad9f)

previously allocated by thread T1 here:
    #0 0x7fbac0ed806b in operator new(unsigned long) (/home/bastiaan/obj-gnash-clang-sanitize/gui/.libs/lt-gtk-gnash+0x6e06b)
    #1 0x7fbac05a05b5 in void std::vector<boost::intrusive_ptr<gnash::SWF::ControlTag>, std::allocator<boost::intrusive_ptr<gnash::SWF::ControlTag> > >::_M_emplace_back_aux<boost::intrusive_ptr<gnash::SWF::ControlTag> const&>(boost::intrusive_ptr<gnash::SWF::ControlTag> const&) /bin/../lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/bits/vector.tcc:412:22
    #2 0x7fbac058ca71 in gnash::SWFMovieDefinition::addControlTag(boost::intrusive_ptr<gnash::SWF::ControlTag>) /home/bastiaan/gnash/libcore/parser/SWFMovieDefinition.h:274:9
    #3 0x7fbac03693f9 in gnash::SWF::DoInitActionTag::loader(gnash::SWFStream&, gnash::SWF::TagType, gnash::movie_definition&, gnash::RunResources const&) /home/bastiaan/gnash/libcore/swf/DoInitActionTag.h:96:9
    #4 0x7fbac05802fd in gnash::SWFParser::read(long) /home/bastiaan/gnash/libcore/parser/SWFParser.cpp:96:17
    #5 0x7fbac0585738 in gnash::SWFMovieDefinition::read_all_swf() /home/bastiaan/gnash/libcore/parser/SWFMovieDefinition.cpp:467:18
    #6 0x7fbabd0b2d9f (/lib64/libstdc++.so.6+0x3b79ebad9f)

Thread T1 created by T0 here:
    #0 0x7fbac0f2753f in pthread_create (/home/bastiaan/obj-gnash-clang-sanitize/gui/.libs/lt-gtk-gnash+0xbd53f)
    #1 0x7fbabd0b2ed8 in std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>) (/lib64/libstdc++.so.6+0x3b79ebaed8)

SUMMARY: AddressSanitizer: heap-use-after-free /usr/include/boost/smart_ptr/intrusive_ptr.hpp:162 boost::intrusive_ptr<gnash::SWF::ControlTag>::operator->() const
Shadow bytes around the buggy address:
  0x0c327fff9280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c327fff9290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c327fff92a0: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c327fff92b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c327fff92c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c327fff92d0: fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c327fff92e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c327fff92f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c327fff9300: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c327fff9310: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c327fff9320: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  ASan internal:           fe
==24477==ABORTING


So this clearly shows data deleted by the thread running read_all_swf() being accessed by another thread.

(file #32996)

Bastiaan Jacques <bjacques>
Group Member
Tue 03 Jun 2014 01:13:45 PM UTC, comment #9: 

For sure you can't load if you hadn't parsed.
What would "loaded" mean otherwise ? How can you know you loaded a frame if you hadn't parsed the code to recognize "nextframe" calls ?

Compare bytes loaded (pre-parse) with frames loaded (post-parse).


Sandro Santilli <strk>
Group Member
Fri 30 May 2014 03:53:33 PM UTC, comment #8: 

I presume you mean MovieClip._framesloaded. But that says nothing about parsing, it only tells you when a frame is loaded, unless you assume that loading happens immediately after parsing. (Which might be true: I don't know, and I doubt it would be possible to be sure without disassembling the pp.) In any case, I think it's not that important to know when parsing happens; from the perspective of the SWF movie, presumably only loading is of importance.

Bastiaan Jacques <bjacques>
Group Member
Fri 30 May 2014 03:38:26 PM UTC, comment #7: 

There's _frames_loaded (or similar, check out the existing tests in actionscript.all) for checking parsing. It takes being able to control size of each frame.

Sandro Santilli <strk>
Group Member
Fri 30 May 2014 12:26:21 PM UTC, comment #6: 

How can you test at which rate the pp parses SWF data? I can see it would be straightforward to test when the data source is being read (querying getBytesLoaded might give a fair approximation), and at which rate, and even when the pp finishes loading certain things into the movie, but actually measuring parsing I can't figure out.

Bastiaan Jacques <bjacques>
Group Member
Thu 29 May 2014 05:12:32 PM UTC, comment #5: 

We know from tests that the parsing is expected to happen in chunks of 64kb per frame advance (or heartbeat? can't remember).

The tests could be improved, but the point is that there's a predictable amount of data being parsed when the file is read from the filesystem.

When I tried to understand the behavior it really looked like an attempt to read ina 64kb buffer w/out waiting for data to be available or not. Being on the filesystem, those are always available and remain that much until next frame (hints to write a test).

I'm mentioning this becase I'd love to see improved compatibility with existing content out there that rely on this characteristic to "assume" the data is loaded in a specific way.

Other than compatibility, the current threaded model worked fine so I see no reason to change it. I mean, if it's not to obtain something better.

Whatever intermediate step that can take us to the better compatibility is welcome. A push parser would be lovely.
Moving the parser to the main thread isn't bad either, but then it would still need a way to only use what's available, as it cannot block. At that point, making it parse "at most 64k per call" would make it even to the compatibility point.

Please keep an eye on tests whatever you do, and consider writing more. I'm pretty sure we don't have much for the 64k as it'd take a big SWF, and don't remember we ever built it on purpose.

Sandro Santilli <strk>
Group Member
Thu 29 May 2014 02:25:11 PM UTC, comment #4: 

If the parsing is moved to the main thread, then the loaders  may have to be converted to use a push rather than pull model for the raw data, which could be a significant undertaking. Alternatively, some sort of threaded loading could be built into SWFStream itself, which I expect would be simpler.

As an alternative to moving parsing into the main thread, the loaders could be modified to return their parsing results (rather than modifying SWFMovieDefinition directly). Then the parsing results would have to be marshalled back to the main thread regularly.

In either case, there would have to be some point at which the either the parsing is actually done or the parsing results are loaded into the main thread, which owns SWFMovieDefinition. Presumably this could happen in ensure_frame_loaded().

It seems obvious to me that reading from SWFStream (which typically involves downloading stuff) should be asynchronous, but I'm not so sure about the parsing done in the loaders. I think we might be able to get away with doing that synchronously, but I'm not sure.

What do you think?

Bastiaan Jacques <bjacques>
Group Member
Mon 26 May 2014 10:29:39 AM UTC, comment #3: 

So far as I'm aware any asynchronous I/O design would still have to have some sort of synchronization in place. But having the parsing (not the I/O) in the main thread would eliminate the issues mentioned in this bug.

Bastiaan Jacques <bjacques>
Group Member
Mon 26 May 2014 09:42:09 AM UTC, comment #2: 

The more elegant solution would be to stop using threads for loading and use async IO instead...

Sandro Santilli <strk>
Group Member
Sat 24 May 2014 11:09:21 PM UTC, comment #1: 

I suppose one way to deal with this issue is to add mutex locks everywhere. I hope there is a more elegant solution available.

Bastiaan Jacques <bjacques>
Group Member
Sat 24 May 2014 11:06:25 PM UTC, original submission:  

read_all_swf() is normally called from a separate thread. It parses the SWF and calls tag loaders for each tag encounters. The tag loader functions receive a reference to 'this' (SWFMovieDefinition object). The trouble is that the loaders call a host of different functions from the SWFMovieDefinition object, some of which are access data members that are not mutex-locked. A quick survey of the functions called are:

Mutex-protected:
  addDisplayObject
  getDefinitionTag

Not mutex-protected:
  addControlTag*
  add_font
  get_font
  get_sound_sample
  getBitmap
  get_loading_sound_stream_id

  • addControlTag actually had a partial mutex protection for the data members access there, by being used in conjunction with _frames_loaded, which had its own mutex; although the access of m_playlist in read_all_swf was unprotected. In any case, the _frames_load mutex was removed in 869f2fa5be27.


Of these, add_font and addControlTag are the most serious, as data is written by the thread to class members in those functions. I'm not sure of the impact of the lack of mutex protection of the members read in the other functions.

See also bug #32569.

Bastiaan Jacques <bjacques>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #32996:  googlecrap.swf added by bjacques (64KiB - application/vnd.adobe.flash.movie)

 

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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-02-04 bjacques Attached File- Added googlecrap.swf, #32996
        Severity3 - Normal 4 - Important
        Summarypotential data races from SWFMovieDefinition::read_all_swf() data races from SWFMovieDefinition::read_all_swf()

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code