search

hohav / peppi

Rust parser for Slippi SSBM replay files
MIT License
37 stars 9 forks source link

Parsing replay files can panic in debug mode. #19

Closed Kered13 closed 1 year ago

Kered13 commented 2 years ago

Line 829 in serde/de.rs can overflow and panic if the replay file is corrupted. The function should return a ParseError instead. I have had this happen on actual replay files that were generated by Slippi, so it doesn't even require random garbage to trigger (I'm not sure why they are corrupted, but I have dozens of such files from playing thousands of games).

There may be other possible places that could panic, but I have not checked the entire parsing code. Any unchecked arithmetic is a potential overflow though. On these particular files it only panics if I am skipping frames though.

Example file attached. Note however that I'm not asking why the file doesn't parse, I know it's corrupt. I'm just saying that the parsing functions should not panic.

corrupt.zip

hohav commented 2 years ago

Thanks for the report. You're right, we shouldn't panic on corrupt files. I should probably implement some basic fuzzing after fixing the obvious issues.

NickCondron commented 1 year ago

Seems like this happens when the raw_len field of the replay is 0. Subtracting bytes_read from 0 causes this panic. raw_len is supposed to be set to 0 for in-progress replay files, but something is failing preventing slippi from updating the 0 to a correct length.

The replay is corrupted, but through the lens of the Read trait, it may be impossible to distinguish between a corrupt replay with raw_len equal to 0 and a normal in-progress replay. I guess we will eventually encounter EOF when parsing a corrupted replay whereas an in-progress replay will simply block?