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

 
 

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

Submitted by:  Bastiaan Jacques <bjacques>
Submitted on:  Sat 24 May 2014 11:06:25 PM UTC  
 
Category: coreSeverity: 3 - Normal
Release: masterStatus: None
Privacy: PublicAssigned to: None
Open/Closed: Open

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)

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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project Member

 

(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):

     

     

    No Changes Have Been Made to This Item

    Back to the top


    Powered by Savane 3.1-cleanup