Skip to content

Commit eb054a3

Browse files
committed
fix: Tighten WS RFC compliance, OAuth2 reentry safety, and HTTP/CI hygiene
- WsClient: reject CR/LF/NUL in upgrade headers; reply 1002 to peer CLOSE with reserved (1004/1005/1006/1015) or 1-byte payloads; close 1002 on PINGs > 125 bytes and accumulate split PING payloads before PONG echo; raise CONNECTTIMEOUT_MS back to 5000; replace assert(sockfd<FD_SETSIZE) with a release-safe runtime check; add CURLE_OK-with-zero-progress loop guard in sendRaw; route ping() through sendRaw with a 1-byte placeholder; QPointer-guard pollRecv re-entry and the queued errorOccurred/close pair; gate open() on connectThread.joinable() to avoid std::terminate; document idle-only setHeaders contract; add removePostedEvents in dtor - CurlHttpClient: drop CURLOPT_TCP_KEEPALIVE (easy handles are not reused); fail the request when curl_slist_append returns null; QPointer-guard self in checkCompleted so callback-driven destruction is safe; rework writeCallback/headerCallback bound checks to compare in size_t against remaining headroom - OAuth2Client: require a %1 placeholder in the localhost policy and re-validate at link() entry; drop the linkingFailed emit on re-entrant link() calls so the in-flight flow's terminal signal wins; defer the refreshFinished(AuthenticationRequired) emit when refresh() runs on a non-owner thread; clear pendingState_ before generating a new state - HttpRequestInvoker: in dtor, sever both directions of connections and call removePostedEvents to evict queued chain dispatches; document the mutex-after-requestQueue member-order invariant - LocalHttpServer: set SO_SNDTIMEO and loop send() until the full response is delivered; range-check response.size() against INT_MAX before narrowing - plugin-main: check curl_global_init return code; null-guard the settingsDialog show lambda; delete settingsDialog in obs_module_unload - IngressLinkSource: route the reload-stages connect() through guard.data(); log an INFO line when ~IngressLinkSource fires the fire-and-forget DELETE so the deferred cleanup is observable - api-client: log the same fire-and-forget DELETE caveat on terminate() - utils.hpp: raise GetAdaptersAddresses retry cap from 3 to 5 to absorb adapter-state churn between the size probe and data fetch - CMakeLists.txt: pin Qt6 floor to 6.2; relax the libcurl target check to require at least one of libcurl_static / libcurl_object; add rationale comments on the OS_* macro origin and Win32 deps - http-error.hpp: rewrite the Redirect FIXME in English - run-clang-format / run-cmake-format actions: pass --break-system-packages, log the resolved binary path, and anchor the version-pin grep
1 parent 594f991 commit eb054a3

17 files changed

Lines changed: 474 additions & 147 deletions

.github/actions/run-clang-format/action.yaml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,11 @@ runs:
5353
echo ::group::Install clang-format-17
5454
# PyPI `clang-format` (ssciwr/clang-format-wheel) ships the upstream
5555
# LLVM 17.0.3 binary, so pinning the version here pins the tool exactly.
56-
python3 -m pip install --quiet --user 'clang-format==17.0.3'
57-
echo "$(python3 -m site --user-base)/bin" >> $GITHUB_PATH
56+
python3 -m pip install --quiet --user --break-system-packages 'clang-format==17.0.3'
5857
export PATH="$(python3 -m site --user-base)/bin:${PATH}"
58+
echo "Resolved clang-format: $(command -v clang-format)"
5959
clang-format --version
60-
clang-format --version | grep -q 'version 17\.0\.3' || {
60+
clang-format --version | grep -qE 'version 17\.0\.3([[:space:]]|$)' || {
6161
echo "::error::clang-format is not pinned to 17.0.3"; exit 1;
6262
}
6363
echo ::endgroup::

.github/actions/run-cmake-format/action.yaml

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,13 @@ runs:
5151
5252
if (( ${changes[(I)*.cmake|*CMakeLists.txt]} )) {
5353
echo ::group::Install cmakelang
54-
pip3 install 'cmakelang==0.6.13'
54+
python3 -m pip install --quiet --user --break-system-packages 'cmakelang==0.6.13'
55+
export PATH="$(python3 -m site --user-base)/bin:${PATH}"
56+
echo "Resolved cmake-format: $(command -v cmake-format)"
57+
cmake-format --version
58+
cmake-format --version | grep -qE '^0\.6\.13$' || {
59+
echo "::error::cmake-format is not pinned to 0.6.13"; exit 1;
60+
}
5561
echo ::endgroup::
5662
echo ::group::Run cmake-format
5763
./build-aux/run-cmake-format --fail-${{ inputs.failCondition }} --check

CMakeLists.txt

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ include("${CMAKE_CURRENT_SOURCE_DIR}/cmake/common/bootstrap.cmake" NO_POLICY_SCO
44

55
# Fail fast on unsupported hosts (FreeBSD/OpenBSD/etc.) before reaching the curl/mbedtls FetchContent block, which would
66
# otherwise produce a confusing TLS-backend error instead of a clear platform-support message.
7+
# OS_WINDOWS / OS_MACOS / OS_LINUX are set by cmake/common/osconfig.cmake (included via bootstrap.cmake).
78
if(NOT
89
(OS_WINDOWS
910
OR OS_MACOS
@@ -71,8 +72,9 @@ if(OS_LINUX)
7172
URL https://github.com/Mbed-TLS/mbedtls/releases/download/mbedtls-3.6.3/mbedtls-3.6.3.tar.bz2
7273
URL_HASH SHA256=64cd73842cdc05e101172f7b437c65e7312e476206e1dbfd644433d11bc56327)
7374

74-
# CMAKE_POLICY_DEFAULT_<NNNN> survives the nested cmake_minimum_required reset done by curl/mbedtls (cmake_policy(SET)
75-
# does not), letting upstream option() / set(... CACHE) honor our function-local overrides.
75+
# Use CMAKE_POLICY_DEFAULT_<NNNN> (a variable, scoped to this function and inherited by add_subdirectory) so the OLD-
76+
# vs-NEW choice survives the nested cmake_minimum_required(VERSION ...) inside the curl/mbedtls subprojects.
77+
# cmake_policy(SET) is stack-pushed and cleared by that reset.
7678
function(_src_link_setup_mbedtls)
7779
set(CMAKE_POLICY_DEFAULT_CMP0077 NEW)
7880
set(CMAKE_POLICY_DEFAULT_CMP0126 NEW)
@@ -115,8 +117,9 @@ FetchContent_Declare(
115117
URL_HASH SHA256=7b40ea64947e0b440716a4d7f0b7aa56230a5341c8377d7b609649d4aea8dbcf)
116118

117119
function(_src_link_setup_curl)
118-
# CMAKE_POLICY_DEFAULT_<NNNN> survives the nested cmake_minimum_required reset done by curl/mbedtls (cmake_policy(SET)
119-
# does not), letting upstream option() / set(... CACHE) honor our function-local overrides.
120+
# Use CMAKE_POLICY_DEFAULT_<NNNN> (a variable, scoped to this function and inherited by add_subdirectory) so the OLD-
121+
# vs-NEW choice survives the nested cmake_minimum_required(VERSION ...) inside the curl subproject.
122+
# cmake_policy(SET) is stack-pushed and cleared by that reset.
120123
set(CMAKE_POLICY_DEFAULT_CMP0077 NEW)
121124
set(CMAKE_POLICY_DEFAULT_CMP0126 NEW)
122125
# Belt-and-suspenders: refuse find_package() outright for everything we do not ship, so curl cannot accidentally link
@@ -223,10 +226,10 @@ mbedtls FetchContent targets are missing because _src_link_setup_mbedtls() runs
223226
# Xcode generator reads CMAKE_XCODE_ATTRIBUTE_* from the top-level directory at generate time rather than from this
224227
# function's scope, so AppleClang still rejects curl 8.12.1's SIZE_T_MAX uses (-Wambiguous-macro) under -Werror. Force
225228
# the override per target.
226-
if(NOT TARGET libcurl_static OR NOT TARGET libcurl_object)
229+
if(NOT TARGET libcurl_static AND NOT TARGET libcurl_object)
227230
message(
228231
FATAL_ERROR
229-
"curl target names changed — both libcurl_static and libcurl_object are expected; update _src_link_setup_curl() -Werror suppression"
232+
"curl target names changed — at least one of libcurl_static or libcurl_object must be defined; update _src_link_setup_curl() -Werror suppression"
230233
)
231234
endif()
232235
foreach(_curl_target IN ITEMS libcurl_static libcurl_object)
@@ -252,6 +255,7 @@ if(OS_LINUX)
252255
set(SRC_LINK_LINUX_CA_PATH
253256
"/etc/ssl/certs"
254257
CACHE STRING "CA bundle path for Linux TLS (directory → CURLOPT_CAPATH, file → CURLOPT_CAINFO)")
258+
# Allowlist absolute paths to suppress path traversal and shell metacharacter injection into the embedded macro value.
255259
if(NOT SRC_LINK_LINUX_CA_PATH MATCHES "^/[A-Za-z0-9._/-]+$")
256260
message(
257261
FATAL_ERROR
@@ -262,8 +266,10 @@ if(OS_LINUX)
262266
endif()
263267

264268
if(OS_WINDOWS)
265-
# Winsock + Win32 crypto for the curl Schannel backend. FIXME: verify CURL::libcurl_static INTERFACE deps and drop
266-
# redundant entries.
269+
# Winsock + Win32 crypto for the curl Schannel backend. ws2_32 / iphlpapi are also required by plugin code itself
270+
# (utils.hpp uses winsock2 / iphlpapi for GetAdaptersAddresses; local-http-server.hpp uses winsock2 for the OAuth
271+
# loopback server), so they remain mandatory regardless of curl interface deps. FIXME: verify CURL::libcurl_static
272+
# INTERFACE deps and drop redundant crypt32 / bcrypt entries.
267273
target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE ws2_32 iphlpapi crypt32 bcrypt)
268274
endif()
269275

@@ -278,7 +284,7 @@ if(ENABLE_FRONTEND_API)
278284
endif()
279285

280286
if(ENABLE_QT)
281-
find_package(Qt6 REQUIRED COMPONENTS Widgets Core)
287+
find_package(Qt6 6.2 REQUIRED COMPONENTS Widgets Core)
282288
target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE Qt6::Core Qt6::Widgets)
283289
target_compile_options(
284290
${CMAKE_PROJECT_NAME} PRIVATE $<$<C_COMPILER_ID:Clang,AppleClang>:-Wno-quoted-include-in-framework-header

src/api-client.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,7 @@ void SRCLinkApiClient::terminate()
332332
uplink = QJsonObject();
333333
// FIXME: fire-and-forget DELETE is canceled by ~CurlHttpClient on shutdown; replace with
334334
// a sync curl_easy_perform or a bounded QEventLoop pump in a separate PR.
335+
INFO_LOG("synchronous shutdown not yet implemented; uplink may remain on server until session timeout");
335336
deleteUplink(true);
336337
}
337338

src/api-websocket.cpp

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,18 @@ SRCLinkWebSocketClient::SRCLinkWebSocketClient(QUrl _url, SRCLinkApiClient *_api
5353
client = new WsClient(this);
5454

5555
connect(reconnectTimer, &QTimer::timeout, this, [this]() {
56-
reconnectPending = false;
5756
if (started && !client->isValid()) {
57+
// FIXME: WsClient::open() can synchronously emit errorOccurred (e.g. curl_easy_init
58+
// failure). Because reconnectPending is cleared after open() returns, that synchronous
59+
// error path is suppressed (scheduleReconnect early-returns while pending=true) and the
60+
// attempt is not retried. Resolve in a separate PR by routing WsClient::performConnect
61+
// failures through Qt::QueuedConnection or splitting reconnectPending into
62+
// "scheduled" vs "in-flight" flags.
5863
open();
64+
reconnectPending = false;
5965
emit reconnecting();
66+
} else {
67+
reconnectPending = false;
6068
}
6169
});
6270

@@ -103,10 +111,12 @@ int SRCLinkWebSocketClient::reconnectDelayMs() const
103111
{
104112
// Exponential backoff: 10s, 20s, 40s, 80s, 160s, 300s (cap)
105113
// (reconnectCount is incremented before this is called, so the first delay is BASE_DELAY_MS * 2.)
106-
static constexpr int BASE_DELAY_MS = 5000;
107-
static constexpr int MAX_DELAY_MS = 300000;
108-
int delay = BASE_DELAY_MS * (1 << (std::min)(reconnectCount, 6));
109-
return (std::min)(delay, MAX_DELAY_MS);
114+
static constexpr qint64 BASE_DELAY_MS = 5000;
115+
static constexpr qint64 MAX_DELAY_MS = 300000;
116+
// Shift on qint64 to avoid signed-int UB if the shift amount or product ever grows.
117+
const qint64 shift = (std::min)(reconnectCount, 6);
118+
const qint64 delay = BASE_DELAY_MS * (qint64{1} << shift);
119+
return static_cast<int>(std::clamp<qint64>(delay, 0, MAX_DELAY_MS));
110120
}
111121

112122
// Reconnect signal flow:

src/net/curl-http-client.cpp

Lines changed: 38 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ with this program. If not, see <https://www.gnu.org/licenses/>
1818

1919
#include <obs-module.h>
2020

21+
#include <QPointer>
22+
2123
#include "curl-http-client.hpp"
2224
#include "linux-ca-locations.hpp"
2325
#include "../plugin-support.h"
@@ -86,8 +88,12 @@ CURL *CurlHttpClient::createEasyHandle(
8688
QByteArray headerLine = it.key() + ": " + it.value();
8789
struct curl_slist *next = curl_slist_append(headerList, headerLine.constData());
8890
if (!next) {
89-
obs_log(LOG_WARNING, "CurlHttpClient: curl_slist_append failed for header '%s'", it.key().constData());
90-
break;
91+
obs_log(LOG_ERROR, "CurlHttpClient: curl_slist_append failed for header '%s'", it.key().constData());
92+
if (headerList) {
93+
curl_slist_free_all(headerList);
94+
}
95+
curl_easy_cleanup(easy);
96+
return nullptr;
9197
}
9298
headerList = next;
9399
}
@@ -98,6 +104,8 @@ CURL *CurlHttpClient::createEasyHandle(
98104

99105
curl_easy_setopt(easy, CURLOPT_SSL_VERIFYPEER, 1L);
100106
curl_easy_setopt(easy, CURLOPT_SSL_VERIFYHOST, 2L);
107+
// FIXME: Schannel on older Windows 10 builds may negotiate TLS 1.2 only even when MAX_TLSv1_3
108+
// is requested. If the server ever mandates TLS 1.3, revisit the Windows support matrix.
101109
curl_easy_setopt(easy, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_TLSv1_3);
102110
curl_easy_setopt(easy, CURLOPT_PROTOCOLS_STR, "http,https");
103111
curl_easy_setopt(easy, CURLOPT_REDIR_PROTOCOLS_STR, "http,https");
@@ -132,10 +140,9 @@ CURL *CurlHttpClient::createEasyHandle(
132140
}
133141
#endif
134142

143+
// Total request timeout (includes connect, TLS handshake, and the full transfer).
135144
curl_easy_setopt(easy, CURLOPT_TIMEOUT_MS, static_cast<long>(timeoutMs));
136-
curl_easy_setopt(easy, CURLOPT_TCP_KEEPALIVE, 1L);
137-
curl_easy_setopt(easy, CURLOPT_TCP_KEEPIDLE, 60L);
138-
curl_easy_setopt(easy, CURLOPT_TCP_KEEPINTVL, 30L);
145+
// TCP keepalive is unnecessary: easy handles are not reused across requests.
139146
curl_easy_setopt(easy, CURLOPT_PRIVATE, ctx);
140147
ctx->easy = easy;
141148

@@ -278,6 +285,10 @@ void CurlHttpClient::checkCompleted()
278285
int stillRunning = 0;
279286
curl_multi_perform(multi, &stillRunning);
280287

288+
// Guard against the callback synchronously destroying this CurlHttpClient.
289+
// After invoking ctx->callback, accessing any member is unsafe unless the guard is alive.
290+
QPointer<CurlHttpClient> guard(this);
291+
281292
int msgsInQueue = 0;
282293
CURLMsg *msg = nullptr;
283294
while ((msg = curl_multi_info_read(multi, &msgsInQueue)) != nullptr) {
@@ -308,13 +319,19 @@ void CurlHttpClient::checkCompleted()
308319

309320
HttpResponse response{static_cast<int>(statusCode), ctx->responseData, error, ctx->responseHeaders};
310321

311-
// Detach ctx from activeRequests BEFORE invoking the callback so that any
312-
// re-entrant call (e.g. callback issuing a new request) cannot observe a stale
313-
// entry, and so cleanupRequest never runs against a ctx the user resurrected.
322+
// Detach ctx before the callback so a re-entrant cancelAll() / cancelAllSilently()
323+
// cannot double-cleanup this ctx (cancelAllSilently drains via std::move, and both
324+
// paths only iterate the ctxs still in activeRequests at the time of the call).
314325
curl_multi_remove_handle(multi, easy);
315326
activeRequests.removeOne(ctx);
316327

317328
ctx->callback(response);
329+
if (!guard) {
330+
// `this` was destroyed during the callback; ctx was already detached from
331+
// activeRequests/multi above, so release it and exit without touching members.
332+
cleanupRequest(ctx);
333+
return;
334+
}
318335
cleanupRequest(ctx);
319336
}
320337

@@ -340,7 +357,12 @@ size_t CurlHttpClient::writeCallback(char *ptr, size_t size, size_t nmemb, void
340357
return 0;
341358
}
342359
size_t totalSize = size * nmemb;
343-
if (ctx->responseData.size() + static_cast<qsizetype>(totalSize) > CurlHttpClient::MAX_RESPONSE_SIZE) {
360+
// Compare in size_t against the remaining headroom so a huge totalSize cannot
361+
// wrap when narrowed to qsizetype before the bound check.
362+
const size_t alreadyBuffered = static_cast<size_t>(ctx->responseData.size());
363+
// short-circuit: underflow guarded by leading "alreadyBuffered > MAX_RESPONSE_SIZE" check
364+
const size_t remainingBudget = static_cast<size_t>(CurlHttpClient::MAX_RESPONSE_SIZE) - alreadyBuffered;
365+
if (alreadyBuffered > static_cast<size_t>(CurlHttpClient::MAX_RESPONSE_SIZE) || totalSize > remainingBudget) {
344366
obs_log(
345367
LOG_WARNING, "CurlHttpClient: Response exceeded %d bytes limit, aborting", CurlHttpClient::MAX_RESPONSE_SIZE
346368
);
@@ -362,17 +384,20 @@ size_t CurlHttpClient::headerCallback(char *ptr, size_t size, size_t nmemb, void
362384
return 0;
363385
}
364386
size_t totalSize = size * nmemb;
365-
// Aggregate header byte cap: abort when cumulative header bytes (including this chunk)
366-
// exceed MAX_HEADER_BYTES. Mirrors the response-body cap in writeCallback.
367-
ctx->headerBytesTotal += static_cast<qsizetype>(totalSize);
368-
if (ctx->headerBytesTotal > CurlHttpClient::MAX_HEADER_BYTES) {
387+
// Aggregate header byte cap. Compare in size_t against remaining headroom so an
388+
// oversize totalSize cannot wrap when narrowed to qsizetype before the bound check.
389+
const size_t headersSoFar = static_cast<size_t>(ctx->headerBytesTotal);
390+
// short-circuit: underflow guarded by leading "headersSoFar > MAX_HEADER_BYTES" check
391+
const size_t headerBudget = static_cast<size_t>(CurlHttpClient::MAX_HEADER_BYTES) - headersSoFar;
392+
if (headersSoFar > static_cast<size_t>(CurlHttpClient::MAX_HEADER_BYTES) || totalSize > headerBudget) {
369393
obs_log(
370394
LOG_WARNING, "CurlHttpClient: Response headers exceeded %lld bytes limit, aborting",
371395
static_cast<long long>(CurlHttpClient::MAX_HEADER_BYTES)
372396
);
373397
ctx->responseTooLarge = true;
374398
return 0;
375399
}
400+
ctx->headerBytesTotal += static_cast<qsizetype>(totalSize);
376401
QByteArray line(ptr, static_cast<qsizetype>(totalSize));
377402
int colonIndex = line.indexOf(':');
378403
if (colonIndex > 0) {

src/net/http-error.hpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ with this program. If not, see <https://www.gnu.org/licenses/>
2323

2424
enum class HttpError : int {
2525
NoError = 0,
26+
// FIXME: All 3xx codes are mapped to Redirect on the assumption that the SRC-Link API
27+
// never returns 3xx. If endpoints that require redirect following are added later,
28+
// implement caller-side handling with a host allowlist and split 304 Not Modified out
29+
// as a separate NotModified value so conditional GET can be supported. Tracked for a
30+
// follow-up PR.
2631
Redirect, // HTTP 3xx — surfaced because CURLOPT_FOLLOWLOCATION is disabled
2732
ConnectionRefused,
2833
TimeoutError,

0 commit comments

Comments
 (0)