If the server holds the lock while sleeping when shutting down,
connections will not be able to unregister themselves from s.activeConns
after receiving the GOAWAY frame from the server. This will then cause
the server to abort connections unnecessarily after it is done sleeping.
Change-Id: I2bce91785db2d138f7bea3a26311139c6a6a6964
Reviewed-on: https://go-review.googlesource.com/c/net/+/760560
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Per RFC 9000, a Destination Connection ID in a Retry packet is of valid
length as long as it is 20 bytes or shorter. However, when validating
tokens for Retry packets, our server currently makes an assumption that
it will only deal with a Destination Connection ID that is exactly 20
bytes long. As a result, a misbehaving or malicious client can cause our
server to panic.
When our server validates a token for a Retry packet, the nonce is
partially constructed from the Destination Connection ID. Due to our
server's bad assumption, a shorter-than-expected Destination Connection
ID will result in cipher.AEAD.Open being given a nonce of bad length. We
currently use XChaCha20-Poly1305 from x/crypto for our AEAD
implementation, which will panic when given a nonce of bad length.
Fixesgolang/go#78292
Change-Id: Ieb65d8b84bcb5c531bdb46d5c75b13dad9c76eb2
Reviewed-on: https://go-review.googlesource.com/c/net/+/758360
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
This CL is the x/net counterpart to CL 755320.
This test contains a race condition in the server handler:
inHandler <- streamID
<-leaveHandler
We assume that all requests queue reading from leaveHandler in order,
but it is possible for the second request (stream id 3) to arrive at
leaveHandler before the first (stream id 1).
We could fix the race with a judicious synctest.Wait, but rewrite
the test to use serverHandlerCall to manipulate server handlers,
which permits us to precisely pick which request to unblock.
Fixes#69670
Change-Id: I9507d1dba07f7d62bcdc6c9bb67c47466a6a6964
Reviewed-on: https://go-review.googlesource.com/c/net/+/755081
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Nicholas Husin <husin@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
This CL adds RegisterServer and RegisterTransport, which are used to
plug our HTTP/3 implementation into net/http. For now, (inaccurately)
assume that RegisterTransport do not take any configs for simplicity.
Various exported symbols have also been changed to be unexported, since
HTTP/3 will be used only via net/http and will not support direct usage.
For golang/go#77440
Change-Id: I7b9528c193c18f5049da7f497d4259153b8770eb
Reviewed-on: https://go-review.googlesource.com/c/net/+/739881
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
The existing implementation of the Name.unpack method does not check the
length of the domain name until parsing is complete. This allows a
malicious user to supply an unreasonably large name and wastle cycles
parsing. This change moves an equivalent check into the loop during
process to short-circuit if we've created too large of a name.
For golang/go#77540
Change-Id: I4c4bf20c0342825a09cefd9b0b3c0bdce0c80137
Reviewed-on: https://go-review.googlesource.com/c/net/+/750260
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
RoundTrip will now call httptrace.ClientTrace.Got1xxResponse, if any,
when receiving 1xx status response from a peer. This allows our client
and server to use HTTP 103 end-to-end.
Got100Continue and Wait100Continue have also been added to RoundTrip as
they are nearby. The rest of httptrace.ClientTrace will be added in the
future.
For golang/go#70914
Change-Id: Ia7ef7dd026a5390225149da3d76b06a2a372c009
Reviewed-on: https://go-review.googlesource.com/c/net/+/749265
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
TestRoundTripRequestBodyErrors/read_error currently has a small chance
of failing due to a race condition.
Most of the time, within writeBodyAndTrailer which runs concurrently,
when a request body returns an error on Read, rt.abort will be called
with the Read error. Since rt.abort is idempotent, the Read error will
then be propagated and the test would pass as expected.
However, before rt.abort is called with the Read error,
rt.reqBodyWriter.Close is first called, which will close the write
direction of the QUIC stream. This creates a small chance where the main
goroutine which runs RoundTrip can encounter an EOF error due to the
closed stream, and subsequently call rt.abort with a different error
than expected, causing test flakiness.
To fix this, this change makes sure that rt.abort is immediately called
when a request body Read returns an error. This should be a no-op, since
a body Read error has always been prioritized over a potential error
from rt.reqBodyWriter.Close anyways.
Fixesgolang/go#77782
Change-Id: I9ec5acf82dd95ac91963ce5c8ddbf08b49af9508
Reviewed-on: https://go-review.googlesource.com/c/net/+/751280
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
This change makes it easier to move x/net/http2 into std.
Moving the http2 package into std and importing it from net/http
(rather than bundling it as net/http/h2_bundle.go) requires
removing the http2->net/http dependency. Moving tests into
the http2_test package allows them to continue importing net/http
without creating a cycle.
Change-Id: If0799a94a6d2c90f02d7f391e352e14e6a6a6964
Reviewed-on: https://go-review.googlesource.com/c/net/+/749280
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Years back, our HTTP/2 server implementation had a stream accounting bug
that would cause it to improperly report a PROTOCOL_ERROR. In response
to this, we modified our Transport to retry RoundTrip when a RST_STREAM
with PROTOCOL_ERROR was received from a peer.
At this point, this retry logic had outlived its usefulness. Instead, it
might cause issues, e.g. a client that sends a malformed request will
keep retrying repeatedly, despite there being zero chance for the
request to actually succeed.
Fixesgolang/go#77843
Change-Id: Ic043723e3535f68f91db33d8f6bcd7fc2dbce856
Reviewed-on: https://go-review.googlesource.com/c/net/+/750720
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The addition of FramePriorityUpdate (0x10) in RFC 9218 introduced a gap
in the frameParsers array indices (0x0a-0x0f). These indices were
initialized to nil, causing a panic when typeFrameParser accessed them
for unassigned frame types (e.g., ALTSVC 0x0a).
This change adds a nil check in typeFrameParser to safely fallback to
parseUnknownFrame for these unassigned types, preventing the crash.
Fixesgolang/go#77652
Change-Id: I14d7ad85afc1eafabc46417a9fff10f9e0a22446
Reviewed-on: https://go-review.googlesource.com/c/net/+/746180
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Mark Freeman <markfreeman@google.com>
This change implements support for Server to send trailer headers, and
for ClientConn to receive said trailer headers. This is just like
go.dev/cl/743600, but in the opposite direction.
The bulk of the implementation relies on the trailer header encoding and
decoding support that was added to bodyWriter and bodyReader
respectively in go.dev/cl/743600.
For golang/go#70914
Change-Id: I0efded4b1ac3e3c6b9479f18402e02e9e764d4a2
Reviewed-on: https://go-review.googlesource.com/c/net/+/744220
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
This change adds support for ClientConn to send trailer headers, and for
Server to receive trailer headers. Support for the other direction will
be done in a follow-up change.
The encoding and decoding of trailer headers are done using bodyWriter
and bodyReader respectively, as these logic are agnostic to whether the
trailer header is sent by the client or the server.
For golang/go#70914
Change-Id: I646d193ae1bc44ddea69b8397d4473d3b11eddf2
Reviewed-on: https://go-review.googlesource.com/c/net/+/743600
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
In HTTP/3, zero or more DATA frames can come after a HEADERS frame to
represent a request or response body. Our current implementation can
behave rather badly when zero DATA frame is sent.
ClientConn does not close the write direction of the stream when it has
no body to send. As a result, our Server can end up reading the next
frame after a HEADERS frame, only to hang infinitely until the timeout
is reached. To fix this, when there is no body to send, ClientConn now
closes the write direction of the stream as soon as it has finished
writing its HEADERS frame. Server will also prevent itself from reading
the stream if a Content-Length header with the value 0 is received.
In the opposite direction (client reading response from a server), a
similar problem also exists, with a slight variant. While our Server
reliably closes its write direction of the stream as soon as the server
handler exits, a problem can still occur when a client receives an empty
response body due to sending a HEAD request. In this case, if the client
decides to read the response body, bodyReader might throw an error due
to a mismatch between the Content-Length header given by the server and
the actual body length. This is fixed by making ClientConn aware that
HEAD requests will always result in an empty response body.
For golang/go#70914
Change-Id: I1e8970672e7076c9dbf84aec8808632d04bac807
Reviewed-on: https://go-review.googlesource.com/c/net/+/742960
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
While running net/http tests against our HTTP/3 implementation locally,
some tests fail due to slight behavior differences in responseWriter
compared to other http.ResponseWriter implementations:
- responseWriter does not return a 200 OK response if a server handler
is completely empty.
- responseWriter does not have a Flush method, and therefore does not
implement http.Flusher.
There are surely more differences, but these are straightforward to fix
right now.
For golang/go#70914
Change-Id: Ieb729a4de4ccb55d670eac2369e73c240b9ac8f8
Reviewed-on: https://go-review.googlesource.com/c/net/+/741720
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Transport currently deadlocks when receiving a WINDOW_UPDATE for a
non-zero stream that increments its window beyond the 2^31-1 bytes
limit.
This is because endStreamError is called to end the non-zero stream,
which tries to lock an already-locked mutex. Therefore, create and use
endStreamErrorLocked instead, which assumes the mutex is already locked.
Fixesgolang/go#77331
Change-Id: Iea212f49a1f305d1bddefb8831dbaca00840870c
Reviewed-on: https://go-review.googlesource.com/c/net/+/739700
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Currently, we are passing a very barebone http.Request to the server
handler: we only initialize an empty http.Request and put whatever info
we can get while decoding QPACK headers.
Unfortunately, this causes the Server to panic when parsing requests
whose headers are meant to be written to http.Request.URL, as
http.Request.URL was never initialized.
Therefore, make sure that http.Request.URL is initialized. Also,
populate other http.Request fields that we can easily figure out as of
now.
For golang/go#70914
Change-Id: Ie6552d6678b430fe4b51069616c0e366791c4e34
Reviewed-on: https://go-review.googlesource.com/c/net/+/738880
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
As part of adding support for HTTP/2 stream prioritization, a
DisableClientPriority field will be added to http.Server to allow users
to completely disable client prioritization if desired. When
DisableClientPriority is set to true, HTTP/2 server will revert back to
the old behavior where streams are processed in a round-robin manner.
For golang/go#75500
Change-Id: Ida083b3ac17a953e5ddb3ad7ab8a81f9cde2bfc1
Reviewed-on: https://go-review.googlesource.com/c/net/+/737521
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
internal/http3 currently depends on internal/quic/quicwire only for its
SizeVarint when writing SETTINGS frame. This makes bundling
internal/http3 into std more complicated since it forces us to also
bundle internal/quic/quicwire and do import remapping. This CL breaks
this dependency for easier bundling.
Dependency on internal/quic/quicwire in test codes have been left alone
since they do not affect bundling into std, and uses more than just
SizeVarint.
Change-Id: I2b85c5487ae6cce422deb4509f68f932d3f0de6b
Reviewed-on: https://go-review.googlesource.com/c/net/+/738060
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
