Audit & hardening: fix memory-safety/UB defects, add tests, sanitizers & static analysis to CI - #1
Merged
Conversation
This addresses three defects in the platform-independent code, all verified locally (the core builds clean under -Wall -Wextra -Wpedantic and the new tests pass under -fsanitize=address,undefined): * C API buffer overflow (rtaudio_c.cpp): the error-callback used strncpy(errmsg, text.c_str(), text.size() - 1). The length was taken from the source string, so a message longer than the 512-byte buffer overran it, and an empty message made size()-1 wrap to SIZE_MAX (AddressSanitizer reports negative-size-param). The buffer was also left unterminated on truncation. The copy now goes through a bounded, always-NUL-terminating helper (rtaudio_c_private.h). * Undefined behavior in convertBuffer (RtAudio.cpp): several integer format conversions left-shifted negative signed values (e.g. `out <<= 24`, `(Int32)(in << 16)`), which is UB before C++20. These now shift through an unsigned type, which is value-preserving for the existing non-negative cases and well-defined for negatives. * VLA in convertCharPointerToStdString (RtAudio.cpp): `char dest[MB_CUR_MAX]` is a variable-length array because MB_CUR_MAX is a runtime expression in glibc. Replaced with the compile-time constant MB_LEN_MAX. Also fixes a stray `if` (should be `else if`) in the SINT8 output conversion chain. New tests/unittest.cpp is a dependency-free test (runs under the dummy API on every platform) covering the error-message copy edge cases (empty/fit/truncate/embedded-NUL/degenerate buffers) and the C++ and C API-name lookup tables, wired into the CMake, autotools and meson test suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHbT36gMxx7WH2tzZfiqUt
Establishes the missing analysis infrastructure on top of the existing build matrix: * Sanitizers job: Debug build with -fsanitize=address,undefined and the test suite run under ctest. Catches leaks and UB on the portable paths (the registered tests need no audio device). * StrictWarnings job: builds the API-independent (dummy) configuration with -Wall -Wextra -Wpedantic -Werror on both GCC and Clang, locking in a warning-clean, well-defined portable core. Verified to pass locally for the library and all example programs. * StaticAnalysis job: runs clang-tidy (driven by a curated .clang-tidy with high-signal correctness/safety checks) and cppcheck. The mature backends still have outstanding findings, so this job is informational (continue-on-error) for now while the output is surfaced in the logs. Build-system changes: * CMake now enables -Wall -Wextra on GCC/Clang and adds an opt-in RTAUDIO_WARNINGS_AS_ERRORS option (used by CI). The previous behaviour of silently adding -Werror in Debug builds is removed so downstream Debug builds are not broken by new warnings; -Werror is now explicit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHbT36gMxx7WH2tzZfiqUt
These address clear-cut, unambiguous defects on error/teardown paths. They are gated behind platform #ifdefs and are compile-verified by the corresponding CI jobs (CoreAudio on macOS, JACK/PulseAudio on Linux); the portable build remains warning-clean and passes its tests. * CoreAudio closeStream(): the condition-variable teardown (pthread_cond_signal/destroy + delete handle) ran unconditionally while every other use of `handle` in the function is guarded by `if (handle)`. Reached via probeDeviceOpen()'s error path before apiHandle is set, this dereferenced a NULL handle. Now guarded. * JACK probeDeviceOpen(): jack_get_ports() returns NULL when no ports match, but the result was indexed (ports[firstChannel]) without a NULL check. Now checks `ports` first. * PulseAudio probeDeviceOpen(): the freshly allocated handle leaked on almost every open failure. An inner `PulseAudioHandle *pah` shadowed the function-scope pointer, and the error label only freed when callbackInfo.isRunning was true, which is never the case at that point. The handle is now published only once fully constructed, and the error path frees the handle (including its pa_simple streams and condition variable) when this call allocated it, while leaving a handle shared with an already-open direction for closeStream() to own. * CoreAudio probeDeviceInfo(): malloc() results were used without a NULL check and CFStringGetCString() failures left the buffer uninitialized before strlen(). Both name lookups now check the allocation and pre-terminate the buffer so a failed conversion yields an empty string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHbT36gMxx7WH2tzZfiqUt
Records the full memory-safety/correctness/portability audit, the Phase 0 tooling/CI additions, the Phase 1 fixes, and the higher-risk findings deliberately deferred for on-hardware validation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHbT36gMxx7WH2tzZfiqUt
Backend changes (compile-verified by the macOS and MinGW CI jobs; not runtime-tested here for lack of the target hardware). The portable build stays warning-clean and its tests pass. * CoreAudio streamDisconnectListener() no longer calls closeStream() directly on the property-listener thread, which removes property listeners from within a listener (deadlock-prone) and frees state the audio callback is using. It now spawns a detached teardown thread, mirroring the JACK disconnect path. * WASAPI WasapiResampler constructor checked no COM HRESULTs, so an unavailable resampler MFT produced a NULL interface that was dereferenced on the realtime audio thread. The interface pointers are now guarded, an isValid() accessor is added, the destructor is guarded, and Convert() falls back to a passthrough copy when the transform is absent. * WASAPI WasapiResampler::Convert() now clamps the resampler output copy to the caller's output buffer capacity, preventing an overrun when the resampler emits more frames than requested at chunk boundaries. * WASAPI ring-buffer indices (inIndex_/outIndex_) are now std::atomic<unsigned int>; they are written by one thread and read by the other, so the previous plain ints were a data race. * DirectSound probeDeviceOpen() leaked the IDirectSound(Capture) device and its buffer on open failures occurring after the objects were created but before they were stored in the handle (the error path only released via the handle). The error path now releases the still-owned objects, and they are cleared once ownership transfers to the handle. AUDIT.md updated to move these items to fixed and note the remaining deferred work (notably making stream_.state atomic across all backends). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHbT36gMxx7WH2tzZfiqUt
This was referenced Jun 23, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Overview
A memory-safety / correctness / portability audit of RtAudio and a first pass of fixes, plus the CI/test infrastructure to keep the library hardened going forward. Full findings and fix status are tracked in
AUDIT.md.All 16 CI jobs pass on this branch, including the new ones. Portable fixes were verified locally under
-fsanitize=address,undefined; backend fixes are compile-verified across the platform matrix (CoreAudio on macOS, ALSA/Pulse/JACK/OSS on Linux, WASAPI/ASIO/DirectSound via MinGW) but, lacking the target hardware, were not runtime-tested — please review the backend changes with that in mind.Phase 0 — test & analysis infrastructure
tests/unittest.cpp: a dependency-free unit test (runs under the dummy API on every platform), wired into the CMake, autotools and meson test suites. Previously onlyapinameswas an actual test. Covers the C++/C API-name lookup tables and the C-API error-message copy (including the regression case for the overflow fixed below)..github/workflows/ci.yml):Sanitizers— builds with ASan/UBSan and runs the test suite.StrictWarnings— builds the portable config with-Wall -Wextra -Wpedantic -Werroron GCC and Clang.StaticAnalysis— runsclang-tidy(.clang-tidy) andcppcheck(informational for now while the mature backends still have findings).-Wextraenabled on GCC/Clang; new opt-inRTAUDIO_WARNINGS_AS_ERRORS. The previous behaviour of silently injecting-Werrorinto Debug builds was removed so downstream Debug builds aren't broken by new warnings.Phase 1 — confirmed safety/correctness fixes
rtaudio_c.cpp):strncpy(errmsg, text.c_str(), text.size()-1)overran the 512-byte buffer on long messages and underflowed toSIZE_MAXon empty ones (AddressSanitizer:negative-size-param), and didn't terminate on truncation. Now routed through a bounded, always-terminating helper (rtaudio_c_private.h) with a regression test.convertBuffer: negative-signed left-shifts (pre-C++20 UB) now shift through an unsigned type (value-preserving). Plus a VLA (MB_CUR_MAX→MB_LEN_MAX) and anif→else ifbug in the SINT8 output chain.closeStreamNULL-deref andprobeDeviceInfouncheckedmalloc/CFStringGetCString; JACKjack_get_portsNULL-deref; PulseAudio handle/stream leak on open failure.Phase 2 — realtime-thread & leak defects (compile-verified only)
closeStream()on the property-listener thread (deadlock-prone + races the audio callback); it now spawns a detached teardown thread, mirroring JACK.isValid(), andConvert()clamps its output copy to the caller's buffer (overrun fix).std::atomic(were a plain-int data race).Not included (deferred — see AUDIT.md)
Behavior-changing items that genuinely need on-hardware validation: making
stream_.stateatomic across all backends (~180 sites), the JACK duplex-input error path /jackXrun-after-free, and the ASIO process-global state redesign.🤖 Generated with Claude Code
https://claude.ai/code/session_01WHbT36gMxx7WH2tzZfiqUt
Generated by Claude Code