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 a possible crash when importing an OGG file with zero-length packets #87246

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

bs-mwoerner
Copy link
Contributor

@bs-mwoerner bs-mwoerner commented Jan 16, 2024

Fixes #87076.
Supersedes #87233.

A zero-length memcpy into 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 that AudioStreamPlaybackOggVorbis makes (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 the OST.ogg file from #84712.

@akien-mga akien-mga added bug topic:audio crash cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 16, 2024
@akien-mga akien-mga added this to the 4.3 milestone Jan 16, 2024
…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 bs-mwoerner marked this pull request as ready for review January 16, 2024 22:52
@bs-mwoerner bs-mwoerner requested a review from a team as a code owner January 16, 2024 22:52
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Sorry for the delay reviewing.

@akien-mga akien-mga merged commit 9705ac4 into godotengine:master Feb 14, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Feb 27, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.1.4.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants