From c98dfa588aab405a713a2fa9e502a1fe46587990 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 30 Jun 2026 19:54:52 -0700 Subject: [PATCH 1/2] Fix source/output lifecycle and correctness bugs 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 --- src/moq-output.cpp | 4 ++++ src/moq-source.cpp | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/moq-output.cpp b/src/moq-output.cpp index f669101..0109ecf 100644 --- a/src/moq-output.cpp +++ b/src/moq-output.cpp @@ -134,6 +134,10 @@ bool MoQOutput::Start() auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); if (result < 0) { LOG_ERROR("Failed to publish broadcast to session: %d", result); + // The session connected above; close it so a retry on this same output + // doesn't reuse the stale handle. Its terminal callback releases the + // outstanding-session reference the destructor waits on. + Stop(false); return false; } diff --git a/src/moq-source.cpp b/src/moq-source.cpp index f5870e1..83a3bb9 100644 --- a/src/moq-source.cpp +++ b/src/moq-source.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -227,19 +228,30 @@ static void moq_source_destroy(void *data) // bug or an unaccounted handle): far better to log and proceed than to hang // OBS on source deletion. In normal operation the terminals arrive within // milliseconds and the timeout is never reached. + bool timed_out = false; if (--ctx->refs > 0) { struct timespec deadline; timespec_get(&deadline, TIME_UTC); deadline.tv_sec += 2; while (ctx->refs > 0) { if (pthread_cond_timedwait(&ctx->refs_zero, &ctx->mutex, &deadline) == ETIMEDOUT) { - LOG_WARNING("Teardown timed out with %d MoQ callback(s) still outstanding", ctx->refs); + LOG_WARNING("Teardown timed out with %d MoQ callback(s) still outstanding; " + "leaking ctx to avoid a use-after-free", + ctx->refs); + timed_out = true; break; } } } pthread_mutex_unlock(&ctx->mutex); + // A subscription callback still holds ctx (it references ctx->mutex, + // ctx->refs, ctx->refs_zero). Freeing now would be a use-after-free when + // that callback fires, so intentionally leak instead. This only happens on + // the abnormal timeout path above. + if (timed_out) + return; + bfree(ctx->url); bfree(ctx->broadcast); // Note: frame_buffer is already freed by moq_source_disconnect_locked @@ -250,6 +262,31 @@ static void moq_source_destroy(void *data) bfree(ctx); } +// Relay URLs can embed credentials (userinfo) or a query/path token, and OBS +// logs are frequently shared for support. Reduce a URL to scheme://host[:port] +// for logging so secrets never reach persistent logs. +static std::string redact_url(const char *url) +{ + if (!url || !*url) + return "(null)"; + + std::string s(url); + size_t scheme = s.find("://"); + std::string prefix = (scheme == std::string::npos) ? "" : s.substr(0, scheme + 3); + size_t rest = (scheme == std::string::npos) ? 0 : scheme + 3; + + // The authority ends at the first '/', '?' or '#'. + size_t auth_end = s.find_first_of("/?#", rest); + std::string authority = s.substr(rest, auth_end == std::string::npos ? std::string::npos : auth_end - rest); + + // Drop any userinfo (user:pass@). + size_t at = authority.find('@'); + if (at != std::string::npos) + authority = authority.substr(at + 1); + + return prefix + authority; +} + static void moq_source_update(void *data, obs_data_t *settings) { struct moq_source *ctx = (struct moq_source *)data; @@ -280,7 +317,7 @@ static void moq_source_update(void *data, obs_data_t *settings) // If settings changed and are valid, reconnect if (settings_changed && valid) { - LOG_INFO("Settings changed, reconnecting (url=%s, broadcast=%s)", url ? url : "(null)", + LOG_INFO("Settings changed, reconnecting (url=%s, broadcast=%s)", redact_url(url).c_str(), broadcast ? broadcast : "(null)"); moq_source_reconnect(ctx); } else if (settings_changed && !valid) { @@ -1114,8 +1151,9 @@ static void moq_source_decode_frame(struct moq_source *ctx, int32_t frame_id) sws_scale(ctx->sws_ctx, (const uint8_t *const *)frame->data, frame->linesize, 0, ctx->frame.height, dst_data, dst_linesize); - // Update OBS frame timestamp and output - ctx->frame.timestamp = frame_data.timestamp_us; + // Update OBS frame timestamp and output. OBS expects nanoseconds; libmoq + // delivers microseconds. + ctx->frame.timestamp = frame_data.timestamp_us * 1000; obs_source_output_video(ctx->source, &ctx->frame); av_frame_free(&frame); From b4b1ccc10052e799c215cb0a8a1cee25c701f805 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 30 Jun 2026 20:13:21 -0700 Subject: [PATCH 2/2] moq-source: strip userinfo at the last '@' when redacting URLs 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 --- src/moq-source.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/moq-source.cpp b/src/moq-source.cpp index 83a3bb9..b37e180 100644 --- a/src/moq-source.cpp +++ b/src/moq-source.cpp @@ -279,8 +279,9 @@ static std::string redact_url(const char *url) size_t auth_end = s.find_first_of("/?#", rest); std::string authority = s.substr(rest, auth_end == std::string::npos ? std::string::npos : auth_end - rest); - // Drop any userinfo (user:pass@). - size_t at = authority.find('@'); + // Drop any userinfo (user:pass@). Use the last '@' so an unescaped '@' in a + // password can't leave part of it behind. + size_t at = authority.rfind('@'); if (at != std::string::npos) authority = authority.substr(at + 1);