pcm/rawmidi/ump: fix stack overflow from timespec/time64 mismatch - #158
Conversation
These FFI calls are typed as snd_htimestamp_t (= alsa_sys::timespec), which alsa-sys 0.6.0 widens to 16 bytes on 32-bit glibc targets with a 64-bit time_t. libc::timespec stays 8 bytes there, so the stack buffer we passed by pointer was undersized: an 8-byte stack overflow on affected targets (e.g. 32-bit Raspberry Pi OS).
|
unfortunately this still segfaults on release running the cpal beep example (cpal 0.17.3 with the overflow fix applied, alsa @ 8b9a0c1, alsa-sys at 0.6) |
|
Thanks for testing that so quickly. Before I chase this further: can you confirm you actually rebuilt against my patched Also, could you run a full rebuild with |
I think this is the case, I added an override and it showed up with
Where would it show up? I ran your build command inside the |
|
Try running something like this: Second, just to rule it out, can you try running with |
|
I'm back to building the beep example now, the build isn't finished yet but it's past alsa-sys. All I get with grep is and grepping the console output of the build: edit: nothing new after the build finished, and running with high |
|
@roderickvd Anything else should cause a compile time error and the fact that it does not is a sign something is off here, probably the time64 probe is not working like it should? |
|
is // $ grep define /usr/include/arm-linux-gnueabihf/bits/timesize.h
#define __TIMESIZE 32 |
|
D'oh, thanks for pulling me out of my grogginess. Re-exported it here, and created RustAudio/cpal#1285 to stack with this one. @d3d9 working now? |
|
I've tried to do the same locally already, replacing the occurrences of libc::timespec with a re-exported alsa::timespec, but the issue persists. Do we have to fix the build check first, since it doesn't recognize the time as being 64 bit? The probe expansion contained |
|
I wonder that despite your What's your output of |
|
|
For reference, this is from my |
|
This confirms that the current build-time probe is not robust enough. @d3d9's Sorry for iterating over this, this time-stuff is hard and apparently distro-dependent. What we could do instead is stop guessing from headers or package metadata and just ask the installed |
|
Separately we should then also pin the Rust struct's alignment explicitly with Man, what did I get us into? 😅 (myself anyway) |
Yeah, but let's do that probe runtime (at program startup) instead. Or at snd_pcm/rawmidi_open time. |
|
what if we changed the build time probe to check for the combination of __USE_TIME_BITS64 and __TIMESIZE == 32 like it seems to be the case on my end? |
|
Or, I guess not just that combination. but e.g. The fixes themselves seem to work now by the way, after making sure |
__TIMESIZE reports the port's default time_t width, so it still reads 32 on targets that gained a 64-bit time_t through Debian's time64 transition (_TIME_BITS=64 in the toolchain, e.g. armhf on trixie). On those targets the probe from diwic#19 does not emit alsa_sys_time64, libc::timespec stays 8 bytes, and libasound writes 16-byte timespecs past it (diwic/alsa-rs#158, RustAudio/cpal#1285). Measure the size directly instead: try to compile a _Static_assert that sizeof(struct timespec) == 16. The snippet is compiled but never run, so cross compiling against the target's sysroot keeps working. A control compile first checks that <time.h> yields a usable timespec, so a broken toolchain produces a warning and the safe 32-bit fallback instead of being misread as a pre-transition target. Verified on Raspberry Pi OS armhf: with this change the cfg is emitted on trixie (16-byte timespec) and still not on bookworm (8-byte timespec). 64-bit glibc and musl targets return early as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@d3d9 you're saying that changing the probe to // FFI-facing: opaque and enough bytes+alignment for any ABI layout:
// - 32-bit legacy (i32, i32)
// - 32-bit time64 (i64, i32+pad)
// - 64-bit native (i64, i64)
#[repr(C, align(8))]
#[derive(Copy, Clone, Debug, Default)]
struct RawTimespec([u8; 16]);
// Caller-facing: same type on every target
#[derive(Copy, Clone, Debug, Default)]
pub struct timespec {
pub tv_sec: i64,
pub tv_nsec: i32,
}We'd then resolve the real layout once, at Once the layout is resolved at runtime, should |
Exactly, it does. Technically |
|
@roderickvd @d3d9 Does this fix the issue? diwic/alsa-sys#20 |
This also works on my end |
* fix: measure sizeof(struct timespec) instead of reading __TIMESIZE __TIMESIZE reports the port's default time_t width, so it still reads 32 on targets that gained a 64-bit time_t through Debian's time64 transition (_TIME_BITS=64 in the toolchain, e.g. armhf on trixie). On those targets the probe from #19 does not emit alsa_sys_time64, libc::timespec stays 8 bytes, and libasound writes 16-byte timespecs past it (diwic/alsa-rs#158, RustAudio/cpal#1285). Measure the size directly instead: try to compile a _Static_assert that sizeof(struct timespec) == 16. The snippet is compiled but never run, so cross compiling against the target's sysroot keeps working. A control compile first checks that <time.h> yields a usable timespec, so a broken toolchain produces a warning and the safe 32-bit fallback instead of being misread as a pre-transition target. Verified on Raspberry Pi OS armhf: with this change the cfg is emitted on trixie (16-byte timespec) and still not on bookworm (8-byte timespec). 64-bit glibc and musl targets return early as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: probe snd_htimestamp_t via the ALSA headers and fail on probe errors Review feedback from #20: - Measure sizeof(snd_htimestamp_t) from <alsa/asoundlib.h> instead of struct timespec from <time.h>: it is the exact type libasound writes through. The probe now runs after pkg-config so it can reuse the reported include paths. The alsa/ prefix is required since alsa-lib 1.2.5, when alsa.pc stopped adding -I${includedir}/alsa. - If the control snippet does not compile, fail the build with an explicit error instead of assuming a 32-bit time_t: guessing 8 can segfault and guessing 16 yields garbage timestamps, so there is no safe fallback to pick silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
When you release |
@roderickvd Released now. I might not have time to do much work with this in the coming weeks, so in case this does not work either, let me know ASAP. |
|
Thanks, @diwic. I'll wire up the changes a.s.a.p. on the |
3 participants
Leftover from our
time64work: when we fixed the type in alsa-sys (diwic/alsa-sys#19) we did not switchget_htstamp,get_trigger_htstamp,get_audio_htstamp, and therawmidi/umptread()functions to usealsa::timespecinstead oflibc::timespec.Through cpal's Discord it was reported that when compile-time optimization was turned on, this still caused segfaults on 32-bit Raspberry Pi's. Here the 64-bit
time_tstill diverge (16 vs. 8 bytes) and the remaining 8 bytes probably did not get reused in debug builds.I don't have access to my Linux environment, so I'm asking the Discord user to confirm this is working for him/her first.