Fix junk data in ffprobe

polyfloyd / rust-id3

A rust library for reading and writing ID3 metadata

MIT License

245 stars 47 forks source link

Fix junk data in ffprobe #56

Closed Marekkon5 closed 3 years ago

Marekkon5 commented 3 years ago

Hello, this is my attempt at fixing issue #39 .

I've made a proper file using Kid3 and then compared it to tag from this library and made the following changes:

The padding in file should be 1024, however your code somewhere adds an extra byte, so made it 1023
The header size (and 1 extra byte) isn't included in the good tag, so removed them, however the 1024b padding is included.

I've also added a quiet.mp3 file, which after tagging works in ffprobe and didn't without my changes.

Thank you.

Marekkon5 commented 3 years ago

Hello, I've made changes to the test and added the padding to the encoder builder. I've also included ffmpeg in the Github Actions workflow, so the test can pass.

However I didn't solve the last problem, as I am not really familiar with the entire codebase to do it properly. Hopefully you can fix that and merge soon. Thank you.

Marekkon5 commented 3 years ago

I've actually tested it right now on different (actual music files) and the issue still seems to be present.

Marekkon5 commented 3 years ago

Okay, so this commit was tested on ~1000 MP3 files, all were issue free, so hopefully that's it. However I had to change the assert in storage.rs on line 263, which am not completely sure what does. However without the change it crashes after trying to write multiple times to same file, and with the change I didn't have any issues, so I am assuming it's safe, but let me know. Thank you.

polyfloyd commented 3 years ago

Thanks! I try to schedule some time to push this along the coming weekend :)

polyfloyd commented 3 years ago

Reviewed and merged! Although I have squashed your changes and fixed to off-by-one and storage padding issues in another commit :)

Once again, thank you for your effort!