This test assumed that creating a stream and flushing it on the
client ensured the server had accepted the stream. This isn't
the case; the stream has been delivered to the server, but there's
no guarantee that it been accepted by the user layer.
Change the test to make a full loop: The client creates a stream,
and then waits for the server to close it.
Fixesgolang/go#64788
Change-Id: I24f08502e9f5d8bd5a17e680b0aa19dcc2623841
Reviewed-on: https://go-review.googlesource.com/c/net/+/554175
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The streams map contains nil entries for implicitly-created streams.
(Receiving a packet for stream N implicitly creates all streams of the
same type <N.)
We weren't checking for nil entries when iterating the map on PTO,
resulting in a panic.
Change the map value to be a wrapper type to make it more explicit that
nil entries exist.
Change-Id: I070c6d60631744018a6e6f2645c95a2f3d3d24b6
Reviewed-on: https://go-review.googlesource.com/c/net/+/550798
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
QUIC packet numbers are truncated to include only the least
significant bits of the packet number. The number of bits
which must be retained is computed based on the largest
packet number known to have been received by the peer.
See RFC 9000, section 17.1.
We were incorrectly using the largest packet number
we have received *from* the peer. Oops.
(Test infrastructure change: Include the header byte
in the testPacket structure, so we can see how many
bytes the packet number was encoded with. Ignore this
byte when comparing packets.)
Change-Id: Iec17c69f007f8b39d14d24b0ca216c6a0018ae22
Reviewed-on: https://go-review.googlesource.com/c/net/+/545575
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
We were failing to hold streamsState.streamsMu when removing
a closed stream from the conn's stream map.
Rework this to remove the mutex entirely.
The only access to the map that isn't on the conn's loop is
during stream creation. Send a message to the loop to
register the stream instead of using a mutex.
Change-Id: I2e87089e87c61a6ade8219dfb8acec3809bf95de
Reviewed-on: https://go-review.googlesource.com/c/net/+/545217
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
A receiver that is sending only non-ack-eliciting packets
(for example, a connection reading data from a stream but not sending
anything other than ACKs in response) can accumulate a large amount
of state for in-flight, unacknowledged packets.
Add an occasional PING frame when in this state, to cause the peer
to send an ACK for our outstanding packets.
Change-Id: Iaf6b5a9735fa356fdebaff24200420a280b0c9a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/545215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Do not commit data written to a stream to the network until
the user explicitly flushes the stream, the stream output
buffer fills, or the output buffer contains enough data to
fill a packet.
We could write data immediately (as net.TCPConn does),
but this can require the user to put their own buffer in
front of the stream. Since we necessarily need to maintain
a retransmit buffer in the stream, this is redundant.
We could do something like Nagle's algorithm, but nobody
wants that.
So make flushes explicit.
For golang/go#58547
Change-Id: I29dc9d79556c7a358a360ef79beb38b45040b6bc
Reviewed-on: https://go-review.googlesource.com/c/net/+/543083
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Negotiate the connection idle timeout based on the sent and received
max_idle_timeout transport parameter values.
Set a configurable limit on how long a handshake can take to complete.
Add a configuration option to send keep-alive PING frames to avoid
connection closure due to the idle timeout.
RFC 9000, Section 10.1.
For golang/go#58547
Change-Id: If6a611090ab836cd6937fcfbb1360a0f07425102
Reviewed-on: https://go-review.googlesource.com/c/net/+/540895
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
CL 46008 fixedgolang/go#20704 by not recycling the responseWriterState
if any previous Write call failed, as there could be outstanding
goroutines referencing the responseWriterState memory.
More recently, CL 467355 fixed a variant of the same issue by not
referencing that memory after exiting the Write call. This fix
supersedes the fix in CL 46008, as it is more general and does not
require the caller to know whether any previous Write calls failed.
This CL partially reverts CL 46008 just leaving the test case to ensure
that golang/go#20704 does not regress.
Change-Id: I18ea4d27420265a94cc7af21f1dffa3f7dc3bd34
Reviewed-on: https://go-review.googlesource.com/c/net/+/534315
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Commit-Queue: Damien Neil <dneil@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
After receiving a stateless reset, we must enter the draining
state and send no further packets (including CONNECTION_CLOSE).
We were sending one last CONNECTION_CLOSE after the user
closed the Conn; fix this.
RFC 9000, Section 10.3.1.
Change-Id: I6a9cc6019470a25476df518022a32eefe0c50fcd
Reviewed-on: https://go-review.googlesource.com/c/net/+/540117
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
New server-side conns need to know a variety of connection IDs,
such as the Initial DCID used to create Initial encryption keys.
We've been providing these as an ever-growing list of []byte
parameters to newConn. Bundle them all up into a struct.
Add the client's SCID to the set of IDs we pass to newConn.
Up until now, we've been setting this when processing the
first Initial packet from the client. Passing it to newConn
will makes it available when logging the connection_started event.
Update some test infrastructure to deal with the fact that
we need to know the peer's SCID earlier in the test now.
Change-Id: I760ee94af36125acf21c5bf135f1168830ba1ab8
Reviewed-on: https://go-review.googlesource.com/c/net/+/539341
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
UDP datagrams containing Initial packets are expanded to 1200 bytes
to validate that the path is capable of supporting the smallest
allowed maximum QUIC datagram size.
(In addition, client Initial packets must be sent in datagrams
of at least 1200 bytes, to defend against amplification attacks.)
We were expanding client datagrams containing Initial packets,
but not server datagrams. Fix this. (More specifically, server
datagrams must be expanded to 1200 bytes when they contain
ack-eliciting Initial packets.)
RFC 9000, Section 14.1.
Change-Id: I0c0c36321c055e960be3e29a49d7cb7620640b82
Reviewed-on: https://go-review.googlesource.com/c/net/+/538776
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
When pipe.discardBefore was called with an offset greater
than the current pipe.end position, we would update
pipe.start but not pipe.end, leaving the pipe in an
inconsistent state where start > end. This could then
subsequently cause a panic when writing data that
lies before pipe.start.
This sequence occurs when handling several in-order
CRYPTO frames (where we skip writing in-order data
to the pipe, but still call discardBefore), followed
by an out-of-order frame containing resent data.
Change-Id: Ibac0caad53cd30dac1cd4719a825226809872d96
Reviewed-on: https://go-review.googlesource.com/c/net/+/538775
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
We were failing to add new connections to the listener's set of
live connections, so closing a listener wouldn't abort connections
or wait for them to shut down.
We were also aborting active connections with an error that resulted
in the connection closing with an INTERNAL_ERROR status. Close with
NO_ERROR instead.
For golang/go#58547
Change-Id: I89b6c4fabf744ae5178c0cae655929db1ae40ee4
Reviewed-on: https://go-review.googlesource.com/c/net/+/537935
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Add a StatelessResetKey config field to permit
generating consistent stateless reset tokens
across server restarts.
Set the stateless_reset_token transport parameter
and populate the Token field in NEW_CONNECTION_ID
frames.
Detect reset tokens in datagrams which cannot
be associated with a connection or cannot be parsed.
RFC 9000, Section 10.3.
For golang/go#58547
Change-Id: Idb52ba07092ab5c08b323d6b531964a7e7e5ecea
Reviewed-on: https://go-review.googlesource.com/c/net/+/536315
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Refactor the testConn/testListener relationship some.
Move synthetic time tracking into the listener.
Let the testListener create testConns.
These changes will allow us to test Retry behavior,
where the listener responds to a new connection request
with a Retry packet, and only initiates the connection
upon receiving a valid Retry token.
For golang/go#58547
Change-Id: Ib6fc86a21819059f2a603fa6c9be14ab87a7a44c
Reviewed-on: https://go-review.googlesource.com/c/net/+/535236
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
After CL 534215 was merged to fix a CVE it introduced
an underflow when we try to decrement sc.curHandlers
in handlerDone.
The func startPush calls runHandler without incrementing
curHandlers. Seems to only affect users of http.Pusher.
For golang/go#63511
Change-Id: Ic537c27c9945c2c2d4306ddb04e9527b65cee320
GitHub-Last-Rev: 249fe55f75
GitHub-Pull-Request: golang/net#197
Reviewed-on: https://go-review.googlesource.com/c/net/+/535595
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Connection-level flow control sets a bound on the total maximum
stream offset of all data sent, not the total amount of bytes sent
in STREAM frames. For example, if we send the bytes [0,10) for a
stream, and then retransmit the same bytes due to packet loss,
that consumes 10 bytes of connection-level flow, not 20.
We were incorrectly tracking total bytes sent. Fix this.
We were blocking retransmission of data in lost STREAM frames
on availability of connection-level flow control.
We now place a stream with retransmitted data on queueMeta
(non-flow-controlled data), since we have already
accounted for the flow control window consumption of the
data.
We were incorrectly marking a stream as being able to send
an empty STREAM frame with a FIN bit, when the stream was
actually blocked on stream-level flow control. Fix this.
For golang/go#58547
Change-Id: Ib2ace94183750078a19d945256507060ea786735
Reviewed-on: https://go-review.googlesource.com/c/net/+/532716
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
In Conn.appendStreamFrames, a stream can be moved from the
data queue (for streams with only flow-controlled frames to send)
to the metadata queue (for streams with non-flow-controlled frames
to send) if some other goroutine asynchronously modifies the
stream state.
Adjust the check at the end of this function to clear the needSend
bool only if queueMeta and queueData are both empty, to avoid
losing track of the need to send frames when this happens.
For golang/go#58547
Change-Id: Ib9ad3b01f543cd7673f5233ceb58b2db9adfff5a
Reviewed-on: https://go-review.googlesource.com/c/net/+/531656
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
