Skip to content

Fix five testable crash bugs; add standalone regression suite#71

Open
styler wants to merge 1 commit into
masterfrom
tests/regression-suite
Open

Fix five testable crash bugs; add standalone regression suite#71
styler wants to merge 1 commit into
masterfrom
tests/regression-suite

Conversation

@styler
Copy link
Copy Markdown
Member

@styler styler commented May 22, 2026

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=ON so 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:

cmake -S tests -B tests/build -DSE_ENABLE_TESTS=ON
cmake --build tests/build
ctest --test-dir tests/build --output-on-failure

Bugs fixed

ID File:line Bug
C2 StreamElementsOutput.hpp:345 IsMatchingIndex used return m_index = index; (assignment) instead of ==. Silently rewrote m_index on every call and routed encoders to the wrong output slots in multi-encoder outputs.
C4 StreamElementsApiMessageHandler.cpp:569 context->cefClientId = context->cefClientId; — self-assignment left the field uninitialised; that garbage flowed into every batched child handler's CEF callback id.
C5 StreamElementsApiMessageHandler.cpp:86-91 CefParseJSON return value passed straight into CefListValue::SetValue with no null guard. Trivially crash-triggerable by any client sending malformed JSON.
C6 StreamElementsAudioComposition.cpp:325, 333 Audio encoder accessor guarded with if (index > MAX_AUDIO_MIXES) instead of >=. Index MAX_AUDIO_MIXES passed the check and read one past the end of the encoder array.
C10 StreamElementsApiMessageHandler.cpp:2403-2413 setCurrentProfile API handler was registered twice; the second registration silently overwrote the first via the m_apiCallHandlers[id] = handler; assignment.

Test design

Two complementary executables under tests/:

  1. test_video_encoder_template_demo — behavioural demonstration of C2 using a minimal mirror of the production logic. Documents what correct IsMatchingIndex behaviour looks like (no mutation of m_index, index 0 matchable, dictionary branch returns false without mutation). Useful as a permanent reference for the failure mode.
  2. 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 an int 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:

FAIL: C2: StreamElementsOutput.hpp must not assign to m_index in IsMatchingIndex
FAIL: C4: StreamElementsApiMessageHandler.cpp must not contain `context->cefClientId = context->cefClientId`
FAIL: C5: CefParseJSON result must be null-checked before being passed to SetValue
FAIL: C6: audio encoder bound check must use `>=`, not `>`
FAIL: C10: setCurrentProfile is registered 2 time(s); expected exactly 1

After the fixes in this PR, all invariants pass.

Test plan

  • CI / reviewer can build with -DSE_ENABLE_TESTS=ON and confirm ctest is 2/2 green
  • Confirm the production-build target is unaffected (the new option() defaults to OFF)
  • Sanity-check that the duplicate setCurrentProfile deletion is correct (the two bodies were byte-for-byte identical; only one registration is now wired)
  • Confirm the CEF null-guard substitutes a sane "null" CefValue (which is what every downstream handler already has to tolerate) rather than skipping the argument

Not 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant