diff options
| author | srdusr <[email protected]> | 2024-09-19 23:57:00 +0200 |
|---|---|---|
| committer | srdusr <[email protected]> | 2024-09-19 23:57:00 +0200 |
| commit | aae93b4575e10d223c6cdd8722ca0cce2d47397c (patch) | |
| tree | 73b75f132de70c5a6298c0b2c5de1c1e28ea0d34 /internal/proxy | |
| parent | a02d612ba5efb5f1e79a2f9a0778890e8c3e6acf (diff) | |
| download | mitmux-aae93b4575e10d223c6cdd8722ca0cce2d47397c.tar.gz mitmux-aae93b4575e10d223c6cdd8722ca0cce2d47397c.zip | |
Fix hang and data-race bugs found while re-verifying steps 1-5
Audited every file in the proxy/store/ipc/TUI stack before starting
step 6, per request. Found and fixed three real bugs in already-shipped
code, all confirmed with live tests (including a race-detector build)
rather than just read:
1. No timeout covered the write-request/read-response phase of an
upstream exchange, in either the main proxy path (roundTripH1/
roundTripH2) or Repeater - only the dial itself was bounded. A
server that accepted the connection and then never finished
responding hung the request forever. Fixed with conn.SetDeadline
after a successful dial in both forward() and Repeat() (new
upstreamTimeout constant, 60s). Verified against a real hung TCP
listener: the daemon returned a clean "i/o timeout" error at exactly
60s instead of hanging.
2. ipc.Client shared one connection/encoder/decoder with no locking.
Bubble Tea dispatches each request as its own goroutine, and
viewList's 'r' key doesn't change mode while its loadDetail call is
in flight - pressing it again (or 'enter' on another row) before the
first response arrives calls Get/List/Repeat concurrently on the same
connection, which can interleave JSON on the wire or hand one call
another's response. Fixed with a mutex serializing round trips.
Stress-tested with rapid overlapping key input against a -race build
of both binaries: no warnings, no corruption.
3. The IPC "subscribe" handler only noticed a disconnected client when
the next broadcast's Encode failed - a subscriber that quit while the
daemon was otherwise idle leaked its goroutine and channel
indefinitely. Fixed by reading the connection in the background too,
so disconnection is detected immediately regardless of traffic.
Also removed a dead, misleading parameter: captureResponse took a *teeConn
it was never actually called with (the exact-capture path is handled
directly in forward()), so the branch using it was unreachable.
Re-verified all five prior steps end-to-end against a fresh build:
plain HTTP, HTTPS H1.1/H2/untrusted-CA-rejection, exact vs reconstructed
capture flags cross-checked directly in SQLite, Repeater over both HTTP
and HTTPS, and search (plain text, dotted domains, hyphenated terms,
column filters) - all correct.
Diffstat (limited to 'internal/proxy')
| -rw-r--r-- | internal/proxy/capture.go | 14 | ||||
| -rw-r--r-- | internal/proxy/proxy.go | 14 | ||||
| -rw-r--r-- | internal/proxy/repeat.go | 5 |
3 files changed, 24 insertions, 9 deletions
diff --git a/internal/proxy/capture.go b/internal/proxy/capture.go index ccc95e9..ec4dc8d 100644 --- a/internal/proxy/capture.go +++ b/internal/proxy/capture.go @@ -59,14 +59,12 @@ func captureRequest(r *http.Request, tee *teeConn, bodyCap *cappedTee) (raw []by return buf.Bytes(), false } -// captureResponse mirrors captureRequest for the upstream leg: exact -// wire bytes when tee is non-nil (upstream negotiated HTTP/1.1), -// otherwise a reconstruction. -func captureResponse(resp *http.Response, tee *teeConn, bodyCap *cappedTee) (raw []byte, exact bool) { - if tee != nil { - return tee.Take(), true - } - +// captureResponse reconstructs raw response bytes from the parsed +// response for the HTTP/2 upstream case - the exact-capture path +// (HTTP/1.1 upstream) is handled directly in forward() via the +// teeConn's own Take(), which is the only reason this one doesn't also +// need a *teeConn parameter. +func captureResponse(resp *http.Response, bodyCap *cappedTee) (raw []byte, exact bool) { dump := *resp if bodyCap != nil { dump.Body = io.NopCloser(bytes.NewReader(bodyCap.buf.Bytes())) diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index fcf8c14..014c601 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -38,6 +38,10 @@ import ( "mitmux/internal/store" ) +// upstreamTimeout bounds the write-request/read-response phase of an +// upstream exchange, once dialing has already succeeded. +const upstreamTimeout = 60 * time.Second + // hopByHopHeaders are stripped before forwarding a request or response, // per RFC 7230 6.1 - they are meaningful only between a client and its // immediate next hop, not end-to-end. @@ -259,6 +263,14 @@ func (s *Server) forward(dial dialer, scheme, hostname string, w http.ResponseWr return } defer conn.Close() + // The dial itself is bounded (net.Dialer.Timeout / HandshakeContext); + // without this, a server that accepts the connection and then never + // writes or never finishes writing would hang the request forever - + // there's no other timeout covering the write-request/read-response + // phase. Bounds the whole exchange, so a legitimately slow multi- + // minute transfer would also get cut off; a fixed default is enough + // for now, not worth a config surface yet. + conn.SetDeadline(time.Now().Add(upstreamTimeout)) var resp *http.Response var upstreamTee *teeConn @@ -299,7 +311,7 @@ func (s *Server) forward(dial dialer, scheme, hostname string, w http.ResponseWr if upstreamTee != nil { respRaw, respExact = upstreamTee.Take(), true } else { - respRaw, respExact = captureResponse(resp, nil, respBodyCap) + respRaw, respExact = captureResponse(resp, respBodyCap) } s.record(started, duration, scheme, hostname, r, reqRaw, reqExact, respRaw, respExact, resp.StatusCode, "") diff --git a/internal/proxy/repeat.go b/internal/proxy/repeat.go index 6373ee1..cecf481 100644 --- a/internal/proxy/repeat.go +++ b/internal/proxy/repeat.go @@ -33,6 +33,11 @@ func (s *Server) Repeat(ctx context.Context, scheme, host string, raw []byte) (* return s.recordRepeat(started, time.Since(started), scheme, host, method, path, raw, nil, 0, err.Error()) } defer conn.Close() + // See the matching comment in forward(): without this, a hung + // server - or a user-edited request malformed enough that nothing + // ever replies - blocks this Repeat call, and the IPC connection + // handling it, forever. + conn.SetDeadline(time.Now().Add(upstreamTimeout)) if _, err := conn.Write(raw); err != nil { return s.recordRepeat(started, time.Since(started), scheme, host, method, path, raw, nil, 0, err.Error()) |