Repository navigation
deps: bump libwebsockets v5.0-stable → v5.1.0 (carried patches 12 → 6) - #1198
Conversation
Move deps/libwebsockets from the v5.0-stable pin ac23cae70 to the v5.1.0
tag (5f39aa483) and cut the carried patches from 12 to 4.
- Drop 10 patches that v5.1.0 has upstream, fixed differently or no
longer needs.
- Rebase lws-vhost-inherit-context-log-cx onto lib/io/vhost.c and port
lws-h1-restart-queued-on-connection-close to the sansIO layer.
- Add lws-socks5-empty-address-disables ("" keeps meaning "no socks
proxy", with no socks_proxy env fallback) and
lws-mbedtls-client-no-h3-alpn-over-tcp (h3 stays off TCP handshakes).
- fetch offers "h2,http/1.1,h3": a list without h3 now pins the
protocol against alt-svc, so h3 upgrades need it named.
- tjs.serve turns off lws 5.1's request-body minimum rate on both its
TCP and QUIC vhosts, keeping uploads unlimited as before.
- Turn off the lws 5.1 default-ON options txiki.js doesn't use (GIF,
SVG, MD, HL, CACHE_BLOB, SYS_INFO).
- Tests: a Secure cookie set over http:// is neither stored nor sent;
a socks_proxy environment variable does not divert a direct fetch.
lws 5.1 builds SMD in whenever async DNS is on, and tearing a context
down with an SMD message still pending walks past the last message with
lws_container_of() on a NULL list pointer (offset 0, never dereferenced).
UBSan on macOS flags it as pointer-overflow, aborting
test-teardown-drain-late-fetch{,-port}.
Since upstream bc7cee276, lws tries the shared cookie jar's lock once and skips the jar operation when another process holds it. A lookup then consulted only the jar, so a cookie whose jar write was skipped was never sent back, even in-process (test-xhr-credentials failed on Windows CI). patches/lws-cache-l1-serves-when-jar-busy.patch makes a cache lookup also take the items held in L1, matched with the jar's rules, and answer from L1 alone (cached for at most 1 s) when the jar can't be read. Only persistence is skipped; nothing waits on the event loop. test-fetch-cookie-jar-locked holds the jar lock for a child tjs and checks both a persistent and a session cookie are still sent back, and that nothing was persisted. test-xhr-credentials now runs in a child with an isolated TJS_HOME, so its on-disk check doesn't race other tjs processes in the suite.
saghul
changed the title
|
reviewer (devteam, first pass) on 1de1323: escalated to deep review. No defects found in the first pass. Deep review is required because the card pre-flags it (vendored lws version bump with 5 local patches, including TLS ALPN glue and the cookie cache) and because of size (27 files). Checked locally on macOS x86_64 Release:
|
|
deep-reviewer, round 1 (head 1de1323): changes requested.
Everything else verified: submodule and all 12 cited upstream commits are ancestors of v5.1.0; cache, ALPN (bounds), socks5 and vhost patches read correct; |
The lws-h1-restart-queued-on-connection-close patch scheduled the restart of each request dequeued from a connection that won't be kept alive on the request's sul_timeout. When that request had been aborted in the same service pass (LWS_TO_KILL_ASYNC armed on the same sul, close_decided set), the restart replaced the kill: the aborted request was sent anyway, and with close_decided left set every later timeout on it was ignored. Skip the restart for a dequeued request whose close is already decided and leave the armed kill to close it. Add a regression test that aborts a queued fetch from an I/O callback in the same poll batch as the leader's Connection: close response, over several attempts.
On lws v5.1.0, __lws_wsi_remove_from_sul() leaves close_decided set. The h1 pipeline handoff cancels the retiring leader's sul_timeout that way and then closes it with LWS_TO_KILL_ASYNC, which is ignored if the leader's close was already decided. In txiki.js that is a streaming fetch whose reader is cancelled on the last body chunk while another same-origin request is queued behind it: the retired leader is freed only when the runtime exits. Carry the pin's one-line deviation (clear close_decided there), rebased onto lib/sansio/wsi.c, as a 6th lws patch; upstream PR warmcat/libwebsockets#3685. Add a test that drives that path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
saghul
changed the title
|
deep-reviewer, round 2 (final): approved at head 6e9581b. Both round-1 findings are resolved; nothing else changed since 1de1323 (diff is 5 files: CMake comment, the two patches, two tests), so the round-1 verification of the bump itself still stands. Finding 1 (restart overrides a decided close) — fixed in Finding 2 (retired pipeline leader leaked) — fixed per Saúl's option 1 with
Upstream PR warmcat/libwebsockets#3685 is open (saghul → main, lib/sansio/wsi.c only) and linked in the patch header and the PR table. Also re-checked at this head: submodule at 5f39aa483; exactly 6 Not regressions of this PR, noted for the record: the test files create their temp dirs relative to cwd (pre-existing pattern from |
…unks) (#1202) ## What Remove txiki's own chunked request-body de-chunker from `src/httpserver.c`. Since lws 5.1 (upstream 312615fcb, C-011), lws decodes chunked h1 request bodies itself. `LWS_CALLBACK_HTTP_BODY` carries the decoded payload, and the last-chunk raises `LWS_CALLBACK_HTTP_BODY_COMPLETION`. #1194 put the old de-chunker behind `#if !TJS_LWS_DECHUNKS_REQUEST_BODY` (`LWS_LIBRARY_VERSION_NUMBER >= 5001000`). lws is only ever vendored (CMakeLists.txt has no system-lws option) and #1198 bumped it to v5.1.0, so that code is already compiled out. ## How - Removed the `TJS_LWS_DECHUNKS_REQUEST_BODY` define and every `#if`/`#else`/`#endif` on it. The >= 5.1 branches are now unconditional: - `HTTP_BODY` passes the decoded payload to `tjs_http_emit_body_chunk()`. - `BODY_COMPLETION` on a chunked request ends the body stream with `body_chunk_callback(id, null)`. - Deleted the < 5.1 path and everything only it used: - `tjs_http_chunk_hex_value()` - the `TJSHttpChunkState` enum - the `TJS_HTTP_MAX_CHUNK_*` limits - the `chunk_*` request fields - Kept `req->chunked`, because it still selects streaming the body to JS and `LWS_CALLBACK_CLOSED_HTTP` uses it. - Kept the `Transfer-Encoding` check in `LWS_CALLBACK_HTTP`, which accepts only `chunked` and answers anything else with 400. Its comment now calls it a defence-in-depth backstop. lws already answers other h1 transfer codings with 501, and h2/h3 refuse TE at their framing layer. No behaviour change. Net: 8 insertions, 178 deletions, one file. ## Tests The tests from #1192 and #1194 are unchanged. `test-httpserver-chunked-malformed.js` was written to check both paths. Its comment says the framing is "enforced by txiki's own de-chunker on lws < 5.1 and by lws's ...". The first half of that comment is now historical. The test still checks the only remaining decoder (lws), so I kept it. ## Verification (macOS, local) - `make JOBS=6` (Release): builds with no warnings in httpserver.c. - `./build/tjs test tests/`: 481/481 OK, including `test-httpserver-chunked`, `test-httpserver-chunked-malformed`, `test-httpserver-framing`, `test-fetch-clone-transmit` and `test-fetch-stream-request`. - ASAN build (`cmake -B build-asan -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_WITH_ASAN=ON -DBUILD_WITH_MIMALLOC=OFF`): ran every `tests/test-httpserver-*.js`, `test-fetch-stream-*.js`, `test-serve-*.js` and `test-fetch-clone-transmit.js` with `tjs run`. 41/41 passed, with no ASAN reports. - clang-format 18.1.8 `--dry-run -Werror src/httpserver.c`: clean. Devteam card: t_19497bb1 (lws v5.1 migration 5/5)
What
Bumps the vendored
deps/libwebsocketssubmodule from the v5.0-stable pinac23cae70to the v5.1.0 tag (5f39aa483715b813f4d85a692fcb361b5d8d0c1b). Carried lws patches go from 12 to 6. The PR also includes the txiki changes that only work on v5.1.This is step 4/5 of the lws v5.1 migration. The forward-compatible prep work landed earlier in #1192, #1193 and #1194. After this bump, #1194's
TJS_LWS_DECHUNKS_REQUEST_BODYgate compiles in its ≥ 5.1 branch. The gate and txiki's old de-chunker stay for now; a follow-up removes them.Carried lws patches (6, none merged upstream yet)
lws-vhost-inherit-context-log-cx.patchlib/io/vhost.c(moved by the sans-io split). A vhost created withoutinfo->log_cx, such as lws's internal "system" vhost, still binds to the global log context. Worker runtimes then race on its unlocked refcount (test-tls-worker-concurrent).lws-h1-restart-queued-on-connection-close.patchtest-fetch-connection-close-concurrent). The patch keepsuser_spaceand schedules the dequeued wsi'ssul_timeoutto calllws_header_table_attach(wsi, 0). A dequeued request whose close is already decided (aborted in the same service pass) is not restarted, and its armed kill closes it (test-fetch-connection-close-abort-queued).lws-socks5-empty-address-disables.patch(new)socks_proxy_address = ""fails vhost creation, andNULLwould make lws usesocks_proxyfrom the environment. With the patch,""means no SOCKS proxy and no env fallback. txiki already passes"", so txiki itself doesn't change.lws-mbedtls-client-no-h3-alpn-over-tcp.patch(new)h3pins the protocol against alt-svc. On a non-QUIC connection the patch dropsh3/h3-*before the list goes to mbedtls (test-fetch-alpn-no-h3-over-tcp).lws-cache-l1-serves-when-jar-busy.patch(new)cookies.txt.LCK) once and skips the jar operation when another process holds it; the pin waited up to 5 s. The cookie was still written to the in-process L1 cache, but a lookup only asked the jar, so the cookie was never sent back, even in-process (this failedtest-xhr-credentialson Windows CI). The patch makes a cache lookup also take the unexpired L1 items, matched with the jar's rules, and answer from L1 alone (cached for at most 1 s) when the jar can't be read. Only persistence is skipped; nothing waits on the event loop (test-fetch-cookie-jar-locked).lws-remove-from-sul-clears-close-decided.patch(new). Upstream PR: warmcat/libwebsockets#3685lib/sansio/wsi.c:__lws_wsi_remove_from_sul()also clearsclose_decided. Without it, the h1 pipeline handoff cancels a retiring leader'ssul_timeoutand then closes it withLWS_TO_KILL_ASYNC, and that kill is ignored if the leader's close was already decided. In txiki this happens when a streaming fetch's reader is cancelled on the last body chunk while another same-origin request is queued behind it: the retired leader is freed only when the runtime exits (test-fetch-stream-cancel-last-chunk-queueddrives this path).Dropped lws patches (10)
lws-mbedtls-client-alpn-uaftemp_alpnis now at function scope.lws-h2-migrate-no-follow-redirectlws-windows-fds-racer-ownerlws-win32-connect-check-reschedulelws-h2-client-sid1-gatelws-h2-client-empty-settingslws-h2-client-end-stream-half-closedlws-client-response-connection-closelws-h1-no-queue-on-connection-closelws-close-decided-standslws-remove-from-sul-clears-close-decided.patch(above).All of these commits are ancestors of v5.1.0.
txiki changes
src/httpclient.c):cci.alpn = "h2,http/1.1,h3", with an updated comment. lws only uses a per-connection list. That list must name h3, or lws treats it as pinning the protocol and never upgrades the origin to h3. The new mbedtls patch keeps h3 off the TCP handshake. The module loader ("h2,http/1.1") and the WebSocket client ("http/1.1") don't change, since neither uses QUIC.src/httpserver.c): lws 5.1 cuts a request body that falls belowhttp_body_min_rate(500 B/s after a 20 s grace). Bothtjs.servevhosts, TCP and QUIC, now sethttp_body_min_rate = LWS_HTTP_BODY_MIN_RATE_NONE, so uploads stay unlimited as before.LWS_WITH_GIF/SVG/MD/HL/CACHE_BLOB/SYS_INFO. These default to ON since lws 5.1 and txiki doesn't use any of them. The "Carried fixes for libwebsockets" comment now lists exactly the 6 patches.ubsan.supp): lws 5.1 forces SMD on whenever async DNS is on, and_lws_smd_peer_destroy()doeslws_container_of(NULL)with offset 0 at context teardown (never dereferenced). Added a scopedpointer-overflow:_lws_smd_peer_destroysuppression, like the existing vendored-dependency entries.test-fetch-cookie-secure-over-http(new): aSecurecookie set over http:// is not stored and not sent back. A plain cookie is the control. This test fails on the old pin, as expected.test-fetch-socks-proxy-env-ignored(new): withsocks_proxy=127.0.0.1:<closed port>set for a childtjs, a direct fetch still succeeds. This guards the SOCKS patch.test-fetch-cookie-jar-locked(new): a childtjsruns with its jar's lock file held for the whole run. A persistent and a session cookie set meanwhile are both sent back on the next request (to a deeper path, so the jar's path matching is exercised), and nothing reaches the jar file. Fails on v5.1.0 without the cache patch.test-xhr-credentials: now runs in a child with an isolatedTJS_HOME(tests/helpers/xhr-credentials.js). Otherwise its on-disk check races the other tjs processes the suite runs in parallel against the shared~/.tjs/cookies.txt, since a busy jar now skips persistence.test-fetch-connection-close-abort-queued(new): a fetch queued behind an h1Connection: closeresponse and aborted in the same poll batch is never sent, and every fetch settles. It loops 10 attempts, since the race depends on both fds landing in one poll batch.test-fetch-stream-cancel-last-chunk-queued(new): a streaming fetch whose reader is cancelled on the last body chunk, with a second same-origin fetch queued behind it, 5 times. It checks that both requests share one connection (so the handoff happens), that the queued one is served and that the cancelled one settles. The leak itself can't be seen from JS, so this test passes with or without the patch. What it guarantees is that the retired leader's close now runs on every attempt, including under the ASAN/UBSAN CI jobs. The leak was verified with a debugger (see below).User-visible changes
Transfer-Encodingnow gets 501 instead of 400. It is still rejected before the handler runs.Securecookies from http:// origins are no longer stored or sent.Domain=must domain-match the request host.tjs.servecuts an idle streaming response after 30 s instead of 15 s. The root cause is tracked separately.fetchthrows "TypeError: Network request failed: closed before established" on Windows #1129, warmcat#3552). It has two known regressions: a SERVFAIL for AAAA fails the whole lookup, and a dead or silent resolver takes about 57 s to fail instead of 10 s. Both were accepted and are being reported upstream. There is no local async-dns patch.How verified
macOS 15.7 x86_64, Release builds (
make JOBS=6).origin/masterb40c768 at the old pin:./build/tjs test tests/passes all 471 existing tests. Also run on the pin:test-fetch-cookie-secure-over-httpfails there as expected (found: plain=p1; sec=s1), andtest-fetch-socks-proxy-env-ignoredpasses.origin/master5a3a56f) on v5.1.0: 481/481, with no FAIL and no SKIP.git checkout -- .indeps/libwebsockets, thencmake -B build) printsApplying patch:for all 6lws-*patches. A reconfigure printsPatch already applied:for all 6._lws_generic_transaction_completed_active_conn,lws_sul_wsitimeout_cb,__lws_free_wsiandlws_context_destroy, runningtest-fetch-stream-cancel-last-chunk-queued(5 handoffs):lws-remove-from-sul-clears-close-decided.patch: nolws_sul_wsitimeout_cbafter any handoff, and 6 wsis are freed fromlws_context_destroy. Those are the 5 retired leaders plus the one also freed there in the patched run.lws_sul_wsitimeout_cband the leader's__lws_free_wsi, and 1 wsi is freed fromlws_context_destroy.t5302-probe-leader-abort-handoff.jsshows the same before and after.test-fetch-connection-close-abort-queuedwithout theclose_decidedcheck in the h1 restart patch fails 3/3 runs (the aborted /b was sent). With it, it passes 3/3. The deep-review probet5302-probe-abort-queued-io.jswent from 7/10 runs sending the aborted request to 0/20.test-fetch-cookie-jar-lockedwith the cache patch reverse-applied fails (wanted: kept=k1; sess=s1,found:empty). With the patch it passes.tjs.serveat 10 B/s for 30 s (curl-T -; for h2:--http2 -k, with Content-Length):http_body_min_rate: h1 is cut at about 21 s (curl: (55) Send failure: Broken pipe), and h2 is cut at about 22 s (HTTP/2 stream 1 was not closed cleanly: CANCEL).received 300and status 200 after about 34-35 s.clang-format18.1.8 shows no diff forsrc/httpclient.candsrc/httpserver.c.Residual risk
lws-remove-from-sul-clears-close-decided.patch. It was checked with a debugger, as described above.lws_cache_lookup(). A meta-result miss now also walks all of L1 (every cookie this process holds). A lookup made while the jar is busy is answered from L1 alone for up to 1 s, so a cookie that exists only on disk (written by another process) can be missed during that window.Devteam card: t_5302f73f