Fix a possible crash when importing an OGG file with zero-length packets #87246
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akien-mga
added
bug
topic:audio
crash
cherrypick:4.1
…th packet in an OGG stream. A zero-length memcpy into a null pointer itself does not fail, but for gcc with optimizations, this can cause incorrect code to be generated further down the line since the pointer is then assumed to be non-null. Now stripping zero-length packets and pages without packets from the OggPacketSequence during import. This prevents various warning and error messages for files that end on a zero-length packet.
bs-mwoerner
force-pushed
the
ogg_crash
branch
from
53dc2e9
to
a4db4ae
Compare
akien-mga
approved these changes
Feb 14, 2024
|
Thanks! |
|
Cherry-picked for 4.2.2. |
akien-mga
removed
the
cherrypick:4.2
|
Cherry-picked for 4.1.4. |
akien-mga
removed
the
cherrypick:4.1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #87076.
Supersedes #87233.
A zero-length
memcpyinto a null pointer itself does not fail but apparently causes an optimizing gcc (production build) to generate incorrect code further down the line because it then assumes the pointer to be non-null and skips any conditionals referencing that pointer.Keeping this as a draft until someone with a gcc build pipeline can verify this fix.Edit: Might also fix #84712 since that's the file I used to track down the crash. Not sure about those non-monotonous timestamps reported by ffmpeg, but that file imports and plays fine for me in the Godot 4.3 dev 1 release build if I just skip processing the zero-length packet in the debugger.
Edit 2: Thank you @RichTeaMan for building this with gcc and verifying that it fixes the crash. As for the numerous errors and warnings, I've now changed the importer to completely strip zero-length packets and pages that contain no packets from the
OggPacketSequence. This makes it conform better to the assumptions thatAudioStreamPlaybackOggVorbismakes (such as "after I successfully move to the next packet, I have new data that I can decode"). With this change in place, I don't get any more warnings or errors when importing and playing theOST.oggfile from #84712.