search

open-flash / swf-parser

Rust and Typescript parsers for SWF
GNU Affero General Public License v3.0
28 stars 8 forks source link

Implement parsers for DefineBinaryData tag #70

Closed dmarcuse closed 5 years ago

dmarcuse commented 5 years ago

I've implemented parsers (for both Rust and Typescript) for the DefineBinaryData tag, closing #46. I've done some testing locally, but if you have an SWF you'd like to use for testing this, I can implement a test for it as well.

demurgos commented 5 years ago

Thank you very much. Looks good to me: I'll squash and merge it. I'll publish it during next week.

In the past few weeks, I refactored the lib to improve the tests but I am still trying to figure out how to make it work best.

The tests currently work by dropping files inside the tests directory and they are picked automatically by the test runner. These test samples are shared with other OpenFlash libs (swf-emitter, swf-tree) so instead of being cloned directly in the repo, they are Git submodules (from this org). You can use the local- prefix to add tests that are ignored by Git.

My main issue at the moment is trying to figure out how to let people update the tests while sending a PR. If you used a file without copyright issues you may send it to open-flash-db/movies so it will be used for CI, or if you extracted the tag bytes you can send it to open-flash-db/tags); otherwise just leave it for now.

Did you have some issues while developping / getting things running? Things that could be improved/explained?

demurgos commented 5 years ago

Regarding the reserved bytes, it may be related to https://github.com/open-flash/swf-parser/issues/65: at some point there should be a way to specify if the parser should be strict or loose. At the moment, the parser should default to be less restrictive until there are more test samples to check the expected behavior on invalid input.

dmarcuse commented 5 years ago

I didn't have any issues with setting up for development, but it might be worth clarifying the the project roots (for cargo/npm) are in the rs and ts folders, and the commands to run tests must be run from those folders.

As far as making it more strict goes, I agree with leaving it less restrictive right now. Aside from verifying that an SWF conforms to the specification, I can't think of a case where checking the reserved bytes would be useful at all.

demurgos commented 5 years ago

The main reason I see to check the reserved bytes is to detect the use of undocumented features. If a movie in the wild uses these bytes, it may be worth checking why. These unused bytes can also be used for watermarking so a security focused project may want to be sure that they are effectively zeroed-out.

(PR merged)

dmarcuse commented 5 years ago

Fair enough, in that case would it be worth storing the reserved bytes in the DefineBinaryData struct so that the AST can be reassembled into the same SWF?

demurgos commented 5 years ago

Producing a Concrete Syntax Tree (without any information loss) is not a priority at the moment as it complicates parsing greatly. I think that just checking for validity and providing a way to recover from failures would be a better balance once it comes time to support these kind of use cases.