Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when trying to load an invalid mp3 file #55502

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

DeleteSystem32
Copy link
Contributor

This should fix #55457 by adding an additional check for mp3 file validity.
Additionally, minimp3 is updated to the newest version.

@DeleteSystem32 DeleteSystem32 changed the title Fix crash when loading an invalid mp3 file Fix crash when trying to load an invalid mp3 file Nov 30, 2021
@DeleteSystem32 DeleteSystem32 marked this pull request as ready for review December 1, 2021 00:43
@DeleteSystem32 DeleteSystem32 requested review from a team as code owners December 1, 2021 00:43
@Calinou Calinou added this to the 4.0 milestone Dec 1, 2021
@Calinou Calinou added cherrypick:3.4 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Dec 1, 2021
@akien-mga
Copy link
Member

Looks good! Please also update the commit hash for minimp3 in thirdparty/README.md.

@@ -165,7 +161,7 @@ void AudioStreamMP3::set_data(const Vector<uint8_t> &p_data) {

mp3dec_ex_t mp3d;
int err = mp3dec_ex_open_buf(&mp3d, src_datar, src_data_len, MP3D_SEEK_TO_SAMPLE);
ERR_FAIL_COND(err != 0);
ERR_FAIL_COND(err || mp3d.info.hz == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can be given an explicit error message with the _MSG variant of the macro, as otherwise this might be a bit cryptic to users (especially as this often would happen in a chain reaction of failed loading errors).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I've gone ahead and updated the commit; should hopefully be all good now.

@akien-mga akien-mga merged commit dc4b8f8 into godotengine:master Dec 1, 2021
@akien-mga
Copy link
Member

Thanks!

@DeleteSystem32 DeleteSystem32 deleted the mp3_fixes branch December 1, 2021 15:14
@akien-mga
Copy link
Member

Cherry-picked for 3.5.

I only cherry-picked the library update + the new error check, as the refactoring seemed not to be valid in 3.x, where the data is allocated via the AudioServer: 8a4c583.

For master, I've noticed some differences between the implementation of AudioStreamMP3 initially ported from 3.x and the one of other streams like AudioStreamOGGVorbis, might be worth giving it another check to further sync to the current design of audio streams (e.g. there's no set_data(), get_data() and clear_data(), I guess things are now done differently?).

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 1, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.1.

@DeleteSystem32
Copy link
Contributor Author

For master, I've noticed some differences between the implementation of AudioStreamMP3 initially ported from 3.x and the one of other streams like AudioStreamOGGVorbis, might be worth giving it another check to further sync to the current design of audio streams (e.g. there's no set_data(), get_data() and clear_data(), I guess things are now done differently?).

I was wondering about that as well at first, but I think it's just due to how the new implementation of AudioStreamOGGVorbis works. It stores its data as a OGGPacketSequence now, so the accessors are just named differently. AudioStreamSample still has the raw data accessors as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double-clicking an invalid .mp3 file within Godot's file system crashes the program
3 participants