client: fix pipelining match when host and address differ - #3639
Closed
zzblydia wants to merge 44 commits into
Closed
client: fix pipelining match when host and address differ#3639zzblydia wants to merge 44 commits into
zzblydia wants to merge 44 commits into
Conversation
Basic h.264 / h.265 mkv / mp4 serving to view in browser
quic-version-negotiation qir-last-tests xr-29 qir-l1-fix sai: QIR with QIR_LWS_BRANCH
wt-detrash-qir
lws_system_parse_policy() is compiled whenever LWS_WITH_NETWORK && LWS_WITH_FILE_OPS, but it is implemented with the LEJP JSON parser and referenced lejp_construct()/lejp_parse()/lejp_destruct() unconditionally. Callers such as the versioned-cert / ACME rotation path in tls.c (gated only on LWS_WITH_DIR + LWS_WITH_NETWORK + LWS_WITH_FILE_OPS) do not gate on LWS_WITH_LEJP, so a build with LWS_WITH_LEJP=OFF fails to link with undefined lejp_* symbols. Gate the JSON-parsing implementation on LWS_WITH_LEJP and provide a stub that reports "no policy" (returns nonzero, *_policy = NULL) when LEJP is disabled. Existing callers already treat a nonzero return as "no policy available", so they continue to link and behave correctly without a JSON parser.
When a client mux (h2/h3) connection goes idle between transactions it sits in LRS_IDLING. On reusing it for a new stream, lws_vhost_active_conns() adopts the new stream directly onto the network wsi but left it in LRS_IDLING. LRS_IDLING does not carry LWSIFS_POCB, so lwsi_state_can_handle_POLLOUT() is false for it and the network wsi's POLLOUT is never serviced: the child-walking POLLOUT loop never runs, the new stream's HEADERS are never sent (lws_h2_client_handshake() is never reached), and its response is never read. The request just hangs. Put the revived connection back into LRS_ESTABLISHED (the state it uses while actively muxing) and clear the keep-warm idle timeout, so its POLLOUT is serviced and the queued stream's headers go out. Same fix for the h3/quic branch.
…ring accept If wsi->told_event_loop_closed was set during a manual close, eg lws_libuv_closehandle() or elops_close_handle_manually_uv(), it would remain set even if the wsi is re-used. This causes SSL timeout issues and fd leaks with HTTP -> HTTPS client redirects. Clear wsi->told_event_loop_closed in elops_accept_uv(), to prevent this issue on wsi re-use. It's harmless on a new connection.
When handling LMQCPP_PUBACK_PROPERTIES_LEN_VBI/COMPLETED This vulnerability was discovered by: Anonymous working with TrendAI Zero Day Initiative
lws_wsi_h2_adopt() set client_mux_substream but not mux_substream on a reused (pooled) HTTP/2 client stream, whereas the first stream on a connection carries mux_substream = 1. The HPACK decoder only captures custom / non-indexed response headers into the ah unknown-header list (ah->unk_ll_head) when mux_substream is set, so on a reused client stream those headers are never stored: the unknown-header list is left stale, and lws_hdr_custom_length/copy/name_foreach then walk it and read out of bounds (observed as a heap-use-after-free when the walk runs past the ah into an adjacent freed header table). This also silently drops all custom response headers on the 2nd+ request of a pooled h2 client connection. Set mux_substream on the reused client stream too, matching the first stream, so HPACK captures custom response headers correctly.
A parallel (happy-eyeballs) connect attempt that fails synchronously in connect() (eg, EHOSTUNREACH for an unroutable AAAA result) is cleaned up at the failure site: its socket is closed, its fds-table slot removed and the parallel_conns entry invalidated. Its position_in_fds_table was never assigned (that only happens on the EINPROGRESS path). When the surviving attempt then connects, the completion path iterated all parallel_conns slots up to parallel_count without checking is_valid, calling lws_remove_parallel_fd_safely() on the dead entry: that used the unassigned position (LWS_NO_FDS_POS) to remove an unrelated fds-table slot and double-closed a possibly since-reused fd. Depending on what the scrambled fds table hit, the connection then lost its POLLOUT events (client POST body never sent, transaction stalls to timeout) and context destroy walked a stale entry into a freed wsi (SIGSEGV/SIGBUS). Seen on macOS against dual-stack (A+AAAA) endpoints with no IPv6 route, eg the AWS Kinesis Video *.api.aws hosts. Guard lws_remove_parallel_fd_safely() against invalid/never-registered entries and restore the is_valid check at the connected-path cleanup loop.
The client-side extended CONNECT support had rotted in several ways that each prevented a ws-over-h2 client connection from establishing or passing data: - migrating the original client ask onto h2 sid 1 dropped the ws ask itself: wsi->ws and the do_ws flag stayed on the network wsi, so lws_h2_client_handshake() issued a plain GET instead of CONNECT - the client handshake emitted the ws regular headers (sec-websocket-version / -protocol) BEFORE the :authority pseudo-header; strict peers (including lws itself as server) fail the stream with 'Pseudoheader after normal hdrs' - the requested subprotocol was read from the client's ORIGIN instead of the sent-protocols list - the 200 reply to extended CONNECT has no sec-websocket-accept (there is no key exchange in RFC 8441), and the accept-token confirmation strcmp'd the missing header ptr: NULL deref - the established client stream transitioned to the ws role without LWSIFR_P_ENCAP_H2, so writes / writable requests / tx credit did not route via the h2 parent (the server side sets it; the client didn't) - DATA rx on a client stream carrying ws was delivered as RECEIVE_CLIENT_HTTP_READ http body instead of being run through the ws parser, so ws payload never reached LWS_CALLBACK_CLIENT_RECEIVE With these, lws_client_connect_via_info() with alpn "h2" and no method performs a working RFC 8441 ws client connection against an lws server; covered by the new api-test-ws-h2-txcredit in a later patch.
lws_h2_frame_write() sent DATA regardless of the stream's tx credit
(just logging at info level), relying on the user code to size writes
to lws_get_peer_write_allowance(). But the ws role never exposed a
tx_credit rops, so for ws-over-h2 (RFC 8441) streams that API returned
-1 'no guidance' and user code COULD NOT cooperate even if it wanted
to: any ws server bulk-sending into a stream whose window ran out
overran the peer's advertised window, which strict peers (nghttp2,
browsers) punish by killing the connection with FLOW_CONTROL_ERROR.
Handle it inside lws where the accounting lives:
- lws_h2_frame_write() parks a whole carries-ws DATA frame on the
stream's buflist_out when there is not enough credit (or earlier
frames are already parked, preserving order)
- the POLLOUT servicing loop drains parked frames oldest-first as
WINDOW_UPDATE restores credit (which already re-arms every mux
child). h2 DATA is a byte stream, so a parked frame larger than
the available credit is SPLIT, sending a chunk under a fresh frame
header and rewriting the stored header for the remainder -- the
peer's SETTINGS_INITIAL_WINDOW_SIZE may be permanently smaller than
a parked frame, and waiting for whole-frame credit would deadlock.
Flags (END_STREAM) ride only on a frame's final chunk.
- lws_send_pipe_choked() reports a stream with parked frames as
choked, so user write loops gated on it back off naturally and
parking stays bounded around one frame past the window
- the ws role gets a tx_credit rops delegating to the h2 role when
the stream is h2-encapsulated, so lws_get_peer_write_allowance()
gives real guidance on ws streams and lws_wsi_tx_credit() can grant
manual rx credit (LCCSCF_H2_MANUAL_RXFLOW) on a ws-upgraded stream;
non-encapsulated ws keeps returning -1 as if the rops were absent
Covered by api-test-ws-h2-txcredit in a later patch, which fails on
the previous behaviour ('flow control violated: rx 2048 > granted
1024') and passes with this.
…ted frames The h1 tx path fires LWS_EXT_CB_PACKET_TX_PRESEND from lws_issue_raw_ext_access(), which is where permessage-deflate sets RSV1 (and fixes up the first-frame opcode) on the compressed frame it produced during LWS_EXT_CB_PAYLOAD_TX. The h2-encapsulation path in rops_write_role_protocol_ws() bypassed it entirely, so a ws-over-h2 connection that negotiated permessage-deflate sent DEFLATED payload WITHOUT RSV1: peers dutifully treated the compressed bytes as plain payload and rendered garbage. Observed against Firefox (which offers permessage-deflate on its RFC 8441 extended CONNECT) with the FluffOS mud driver embedding lws; also reproduced with a node http2 extended-CONNECT client. Fire PRESEND on the assembled frame before delegating it to the h2 role, mirroring what the h1 path does.
rops_close_kill_connection_h2() unlinks a carries-ws stream from its h2 parent BEFORE the protocol close callback fires (close_cb runs at __lws_close_free_wsi(), the sibling disconnect already happened in the close_kill_connection rops). User code that reacts to LWS_CALLBACK_CLOSED by trying to schedule a final flush with lws_callback_on_writable() then reaches rops_callback_on_writable_ws() with encapsulation_parent() == NULL: the assert is compiled out on release builds and the deref segfaults (seen in the field as a crash at offset 0x438 == offsetof role_ops, via the FluffOS mud driver when a browser dropped a ws-over-h2 connection with output still queued). Nothing can be scheduled on a stream at that point; return having done nothing instead of crashing.
A ws server vhost and an RFC 8441 h2 ws client in one process. The server bulk-sends a 64KB pattern in 1KB lws_write() chunks gated only on lws_send_pipe_choked(), like typical user code; the client connects with LCCSCF_H2_MANUAL_RXFLOW and drip-feeds tx credit in 1KB WINDOW_UPDATE quanta, forcing the h2 role to park DATA and split frames to fit the window. Fails (within a second) if the connection doesn't come up as genuine ws-over-h2, if the client ever receives more ws payload than it has granted credit for -- only possible when the server ignored the peer's window -- or if the pattern is corrupted / incomplete; passes when the whole transfer arrives in-window. Wired into ctest like the other api-tests.
…ked frames The tx flow-control parking introduced in 57effc6 gates the park, drain and re-arm logic on lws_has_buffered_out(), which for an h2 stream also reports the NETWORK wsi's buflist_out -- the buffer that fills whenever lws_issue_raw() takes a partial socket write. The drain logic, however, assumes the predicate describes the stream's OWN parked frames, which breaks four ways under ordinary socket-level backpressure: - lws_h2_ws_drain_parked_tx(): after the stream's last parked frame is consumed, a partial write left by its own lws_issue_raw(nwsi) keeps the while (lws_has_buffered_out(wsi)) loop alive via the nwsi branch; the next iteration reads the empty stream buflist and returns -1, and rops_perform_user_POLLOUT_h2() closes a healthy stream that had just flushed everything it owned. - rops_perform_user_POLLOUT_h2(): the post-drain re-arm is skipped whenever the nwsi still holds a partial, on the assumption that a WINDOW_UPDATE will re-arm the child. When the peer's window is large (tx credit ample, bottleneck is the socket) no WINDOW_UPDATE is coming, requested_POLLOUT was already consumed, and the pending- writable accounting then drops POLLOUT on the connection: the user callback never fires again and the transfer stalls permanently. - the same gate pulls a SIBLING ws stream -- nothing of its own parked, POLLOUT requested -- into the drain once an earlier child's drain congests the nwsi in the same service pass, hitting the identical empty-buflist close. - lws_h2_frame_write(): the parking predicate fires on nwsi congestion caused by another stream, needlessly deferring fully-credited frames into the drain path instead of letting lws_issue_raw(nwsi) append them in order. Test the stream's own buflist_out directly in all four places, and let a carries-ws child with nothing of its own parked fall through to normal servicing. The existing api-test doesn't catch these because its drip-fed WINDOW_UPDATEs continually re-arm every child and localhost doesn't produce partial writes.
lws_vhost_active_conns() matches a new connection against existing ones by comparing the candidate's adsin argument against each existing wsi's cli_hostname_copy. Before this patch, the caller passed the raw address (CIS_ADDRESS, eg "127.0.0.1") while existing connections had registered themselves using cli_hostname_copy populated from CIS_HOST (eg "127.0.0.1:8081"). Whenever the user's host differs from the address, eg, carrying a :port suffix as usual for a Host: header on a nonstandard port, the strcmp() could never match and pipelining silently fell back to a fresh TCP connection per transaction. Fix by passing wsi->cli_hostname_copy (which already holds the CIS_HOST value) when available, falling back to the raw address. Since cli_hostname_copy originates from CIS_HOST for both new and existing connections, the comparison now matches correctly and keep-alive connections are reused as intended. Co-developed-by: Claude Fable 5 && DeepSeek V4 Pro
|
Member
|
Thanks, it's pushed on main |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am using libwebsockets v4.3.3 as an http client, sending GET requests as a heartbeat to my server over a keep-alive connection.
In
lws_client_connect_infoI setaddress= "127.0.0.1" andhost= "127.0.0.1:8081".From packet capture I found the GET requests were not reusing the tcp connection. After reading the logs
and stepping through the code, I found the match in
lws_vhost_active_connsnever succeeds:because
adsinis taken fromCIS_ADDRESSwhilecli_hostname_copyis taken fromCIS_HOST, so127.0.0.1is compared against127.0.0.1:8081and every transaction opens a new connection.Here is a minimal reproducible example for this issue