Fix source/output lifecycle and correctness bugs#46
Conversation
Four safe, self-contained fixes (surfaced by a CodeRabbit review of the in-tree vendoring in moq-dev/moq#1977): - moq-source: OBS frame.timestamp is nanoseconds but libmoq delivers microseconds, so playback timestamps were 1000x too small, skewing async A/V pacing. Multiply by 1000. - moq-source: redact the relay URL before logging it. URLs can carry credentials/tokens and OBS logs are often shared for support. - moq-source: on the teardown timeout path, leak ctx instead of freeing it. If refs is still nonzero a libmoq callback still references ctx (mutex/cond/refs), so destroying and freeing it is a use-after-free. Leaking on this abnormal path is strictly safer. - moq-output: close the session when moq_origin_publish fails. The session already connected, so returning without closing leaves a live background session and a retry on the same output reuses the stale handle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughThis change modifies MoQ output and source handling in OBS. In moq-output.cpp, a failed publish call now triggers session stop cleanup before returning. In moq-source.cpp, teardown timeout handling avoids freeing context under contention, a new function redacts credentials from URLs before logging, the reconnect log message uses redacted URLs, and video frame timestamps are converted from microseconds to nanoseconds. Changes
Sequence Diagram(s)sequenceDiagram
participant MoQOutput
participant Session
MoQOutput->>Session: moq_origin_publish()
Session-->>MoQOutput: result < 0
MoQOutput->>MoQOutput: Stop(false)
MoQOutput-->>MoQOutput: return false
Related PRs: None identified. Suggested labels: bugfix, security, source Suggested reviewers: None identified. 🐰 A hop, a stop, a session closed, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/moq-source.cpp`:
- Around line 282-285: The userinfo stripping logic in the authority parsing
path is using the first `@`, which can leave part of a password in logs for URLs
handled by `moq::Source` parsing. Update the `authority` cleanup in the same
block to use the last `@` instead, so everything before the final authority
delimiter is removed before logging or further processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a704d74-5472-4545-8ccf-0ded3071207d
📒 Files selected for processing (2)
src/moq-output.cppsrc/moq-source.cpp
find('@') would leave part of a password behind for a URL like
moq://user:p@ss@host; rfind('@') removes everything up to the final
authority delimiter.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pull moq-dev/obs#46 (323d597) into the vendored copy: frame timestamp µs->ns, relay-URL redaction in logs, leak-not-free on the source teardown timeout, and close-session-on-publish-failure. Keeps cpp/obs byte-identical to upstream main (plus the SPDX header). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Four safe, self-contained fixes surfaced by a CodeRabbit review of the in-tree vendoring in moq-dev/moq#1977. Scoped deliberately to the changes that are correct-by-inspection and don't need a live-OBS run to validate; the more invasive lifecycle work is listed under Deferred.
Fixes
moq-source: frame timestamp µs → ns.obs_source_frame.timestampis nanoseconds, but libmoq deliverstimestamp_usin microseconds. We were assigning it directly, so timestamps were 1000× too small, skewing async A/V pacing. (timestamp_usisu64, so* 1000stays in 64-bit.)moq-source: redact the relay URL in logs. The reconnect log printed the full URL, which can carry credentials or a token in userinfo/query. OBS logs are frequently shared for support, so reduce it toscheme://host[:port]. The broadcast name is still logged separately.moq-source: leakctxinstead of freeing it on the teardown timeout. If the 2s wait times out withrefs > 0, a libmoq subscription callback still referencesctx(its mutex/cond/refs). Destroying the primitives andbfree(ctx)anyway is a use-after-free when that callback fires. Leaking on this abnormal path is strictly safer than a UAF.moq-output: close the session whenmoq_origin_publishfails. The session has already connected at that point, so returning withoutStop(false)leaves a live background session; a retry on the same output then reuses the stale handle.Stop(false)closes it and its terminal callback releases the outstanding-session reference.Deferred (need a live-OBS + relay run to validate)
Flagged by the same review but intentionally out of scope here, since getting them wrong risks hangs or new UAFs:
moq-outputteardown UAF on timeout. Unlikemoq-source, OBS owns thedelete, so it can't just leak; the real fix is to move the callback state (mutex/cv/counter) into an independently-owned block that outlivesMoQOutput. That's a refactor worth testing.moq-outputterminal callback should stop the output. A mid-stream relay close should propagate toobs_output_signal_stopso OBS/UI reflect it, without double-signalling the user-initiatedStop()path. Needs care around re-entrancy from the libmoq thread.moq-sourcedecode drain loop.avcodec_receive_frameshould loop untilEAGAIN(andsend_packetEAGAINshould drain-then-resend) so reordered/B-frame output isn't dropped. This wraps a long processing body, so it wants a B-frame stream to validate.Test plan
clang-formatclean.(Written by Claude Opus 4.8)