net/http: fix nil didTimeout deref when a client dial fails - #68
Conversation
| // TINYGO: stored only. The fetch-based round tripper performs its own TLS | ||
| // handshaking (the browser does it), so this bound is not separately | ||
| // enforced — the field exists so std net/http client code that sets it | ||
| // (e.g. skywire's in-tab skysocks client) compiles unchanged. |
There was a problem hiding this comment.
Please do not mention 3rd party solutions when not relevant to the implementation
|
Fixed — the comment now says the field exists so that client code setting it compiles unchanged, without naming anyone. I also went looking for the same thing elsewhere and found one more, on the |
|
@b0ch3nski any more feedback on this PR? |
|
@0pcom can you please rebase this PR against the latest |
42a16f1 to
4079329
Compare
|
Rebased on latest |
Client.do calls didTimeout() on the error path (line 461), and the contract is
that send() returns a non-nil didTimeout whenever err != nil. TinyGo dropped
setRequestCancel but left the roundTrip error path returning the nil named
return value, so any http.Client{Timeout: ...} request whose dial failed (e.g.
connection refused) panicked with a nil func-value dereference instead of
returning the error. Return alwaysFalse on those error paths, matching the
other error returns in send().
Verified: http.Client{Timeout}.Get to a refused port now returns
"connection refused" instead of panicking.
4079329 to
9085777
Compare
|
Down to the one commit, rebased on current The other two added |
|
Thanks @0pcom now merging. |
3 participants
A failed dial panics instead of returning the error.
send()returns the nameddidTimeouton its error paths, but TinyGo droppedsetRequestCancel, so nothing ever assigns it — it is a nilfunc() bool.Client.dothen calls it on exactly that path:so any client request whose dial fails with a deadline set dereferences a nil func value. The error the user should have seen never surfaces.
alwaysFalsealready exists in this file (line 386) for precisely this shape, so the fix is to return it rather than the never-assigned named return. Same on the "nil *Response with a nil error" path just below, which had the same problem.Found while running gRPC over TinyGo, where a refused connection paniced rather than reporting a dial error.