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

test_audio_cvt crash on Windows bundled release #1096

Closed
waych opened this issue Apr 19, 2021 · 3 comments · Fixed by #1098
Closed

test_audio_cvt crash on Windows bundled release #1096

waych opened this issue Apr 19, 2021 · 3 comments · Fixed by #1098
Labels

Comments

@waych
Copy link
Contributor

waych commented Apr 19, 2021

test_audio_cvt is crashing on Windows bundled dynamic build with heap corruption:

process didn't exit successfully: D:\a\rust-sdl2\rust-sdl2\target\release\deps\sdl2-3261d2c91077dec6.exe (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION)
@waych
Copy link
Contributor Author

waych commented Apr 19, 2021

Discussed in #1094 and #1092

@waych
Copy link
Contributor Author

waych commented Apr 19, 2021

Commenting out the call the sys::SDL_ConvertAudio is enough to make the test pass again, suggesting that something is off with this bit of unsafe code.

In fact, the whole thing works correctly for me if I simply add assert_eq!(raw.buf, src.as_mut_ptr()); directly after the call, suggesting that the way the Vec's buffer is used here is not sound.

@Cobrand
Copy link
Member

Cobrand commented Apr 20, 2021

The data conversion may go through several passes; any given pass may possibly temporarily increase the size of the data. For example, SDL might expand 16-bit data to 32 bits before resampling to a lower frequency, shrinking the data size after having grown it briefly. Since the supplied buffer will be both the source and destination, converting as necessary in-place, the application must allocate a buffer that will fully contain the data during its largest conversion pass.

From the docs of SDL_ConvertAudio, that might be the problem. Since we send a rust Vec buffer, if it tries to re-allocate because the sample is too small, that means it expects to deallocate and reallocate our rust buffer using SDL_free and SDL_malloc, causing the error then.

This error doesn't happen on Linux, so this logic must be different depending on the OS we use (which makes sense because audio systems are deeply tied to the OS), which would be why it hasn't been caught until now.

So the solution here would be to change how the buffer sent to SDL_ConvertAudio is allocated, we have to send a SDL_malloc allocated one and never a rust-allocated one.

@Cobrand Cobrand added the bug label Apr 20, 2021
waych added a commit to waych/rust-sdl2 that referenced this issue Apr 21, 2021
This commit rewrites AudioCVT::convert to bounce the audio buffer into a
an SDL2 heap allocation, rather than trying to reuse the rust heap
buffer.  This is critical, as the underlying library warns internally
that the buffer may be reallocated, breaking configurations where
the library is not using the same heap as rust.

The underlying implementation also notes that the underlying buffer may
be transparently resized to larger than the output as part of the
transformations, so relying on the buffer being capacity() bytes long to
ensure the buffer is not re-allocated is a broken assumption.

Closes Rust-SDL2#1096
sypwex pushed a commit to sypwex/rust-sdl2 that referenced this issue Jun 2, 2024
This commit rewrites AudioCVT::convert to bounce the audio buffer into a
an SDL2 heap allocation, rather than trying to reuse the rust heap
buffer.  This is critical, as the underlying library warns internally
that the buffer may be reallocated, breaking configurations where
the library is not using the same heap as rust.

The underlying implementation also notes that the underlying buffer may
be transparently resized to larger than the output as part of the
transformations, so relying on the buffer being capacity() bytes long to
ensure the buffer is not re-allocated is a broken assumption.

Closes Rust-SDL2#1096
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants