Fix five testable crash bugs; add standalone regression suite#71
Open
styler wants to merge 1 commit into
Open
Conversation
Standalone test harness in tests/, opt-in via -DSE_ENABLE_TESTS=ON. No libobs/Qt link; runs in <1s. Each test fails on the unfixed code and passes after the fixes. Bugs fixed: - C2: VideoEncoderTemplate::IsMatchingIndex used `=` instead of `==`, silently mutating m_index and corrupting multi-encoder slot routing. - C4: batchInvokeSeries had `context->cefClientId = context->cefClientId`, leaving cefClientId uninitialised. - C5: CefParseJSON return value was passed to CefListValue::SetValue without a null guard, crashing on any malformed JSON arg. - C6: audio encoder bounds check used `> MAX_AUDIO_MIXES` instead of `>=`, allowing one-past-end array access. - C10: setCurrentProfile API handler was registered twice; the first registration was silently overwritten.
2 tasks
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
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.
Summary
Five concrete crash bugs from the recent crash-bug review, fixed and gated by a new standalone test suite. The tests build and run without libobs, Qt, or an obs-studio tree — they're pure C++17, finish in under a second, and are opt-in via
-DSE_ENABLE_TESTS=ONso production builds are untouched.I verified the bugs by running the suite against the unfixed code (all five invariants fail) and then against the fixed code (all green). Build/run:
Bugs fixed
StreamElementsOutput.hpp:345IsMatchingIndexusedreturn m_index = index;(assignment) instead of==. Silently rewrotem_indexon every call and routed encoders to the wrong output slots in multi-encoder outputs.StreamElementsApiMessageHandler.cpp:569context->cefClientId = context->cefClientId;— self-assignment left the field uninitialised; that garbage flowed into every batched child handler's CEF callback id.StreamElementsApiMessageHandler.cpp:86-91CefParseJSONreturn value passed straight intoCefListValue::SetValuewith no null guard. Trivially crash-triggerable by any client sending malformed JSON.StreamElementsAudioComposition.cpp:325, 333if (index > MAX_AUDIO_MIXES)instead of>=. IndexMAX_AUDIO_MIXESpassed the check and read one past the end of the encoder array.StreamElementsApiMessageHandler.cpp:2403-2413setCurrentProfileAPI handler was registered twice; the second registration silently overwrote the first via them_apiCallHandlers[id] = handler;assignment.Test design
Two complementary executables under
tests/:test_video_encoder_template_demo— behavioural demonstration of C2 using a minimal mirror of the production logic. Documents what correctIsMatchingIndexbehaviour looks like (no mutation ofm_index, index 0 matchable, dictionary branch returns false without mutation). Useful as a permanent reference for the failure mode.test_source_invariants— opens the real production source files and asserts each buggy pattern is absent. Functions as the regression gate. Catches reintroduction of any of the five patterns above without needing the full plugin to build.No vendored test framework; tests use
<cassert>and anint main()returning 0/non-zero. ctest reads the exit code.Verification
Tested on macOS with AppleClang 21. Test suite fails as expected against the unfixed source:
After the fixes in this PR, all invariants pass.
Test plan
-DSE_ENABLE_TESTS=ONand confirmctestis 2/2 greenoption()defaults toOFF)setCurrentProfiledeletion is correct (the two bodies were byte-for-byte identical; only one registration is now wired)CefValue(which is what every downstream handler already has to tolerate) rather than skipping the argumentNot in this PR
The remaining higher-effort findings from the crash-bug review (signal-handler UAF on output stop, canvas-destroyed-before-scenes use-after-free, shared_lock-while-mutating in the output manager, etc.) need real-OBS reproductions and ASan/TSan to verify, so they're left for follow-ups.