Data race assigning peerParams when 0-RTT is used

[ainar-g]

We've been seeing this race in our test code. In short, the tests create a number of streams on a connection and then perform reads and writes simultaneously.

https://go.dev/play/p/OddTBHCi6PM

This is the shortest we've been able to make it. The result of running this with --race (may require a few tries):

WARNING: DATA RACE
Read at 0x00c00084ebc0 by goroutine 808:
  github.com/quic-go/quic-go.(*connection).newFlowController()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:2275 +0x9e
  github.com/quic-go/quic-go.(*connection).newFlowController-fm()
      <autogenerated>:1 +0x3d
  github.com/quic-go/quic-go.(*streamsMap).initMaps.func1()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/streams_map.go:89 +0x9b
  github.com/quic-go/quic-go.(*outgoingStreamsMap[go.shape.interface { CancelRead(github.com/quic-go/quic-go/internal/qerr.StreamErrorCode); CancelWrite(github.com/quic-go/quic-go/internal/qerr.StreamErrorCode); Close() error; Context() context.Context; Read([]uint8) (int, error); SetDeadline(time.Time) error; SetReadDeadline(time.Time) error; SetWriteDeadline(time.Time) error; StreamID() github.com/quic-go/quic-go/internal/protocol.StreamID; Write([]uint8) (int, error); github.com/quic-go/quic-go.closeForShutdown(error); github.com/quic-go/quic-go.getWindowUpdate() github.com/quic-go/quic-go/internal/protocol.ByteCount; github.com/quic-go/quic-go.handleResetStreamFrame(*github.com/quic-go/quic-go/internal/wire.ResetStreamFrame) error; github.com/quic-go/quic-go.handleStopSendingFrame(*github.com/quic-go/quic-go/internal/wire.StopSendingFrame); github.com/quic-go/quic-go.handleStreamFrame(*github.com/quic-go/quic-go/internal/wire.StreamFrame) error; github.com/quic-go/quic-go.hasData() bool; github.com/quic-go/quic-go.popStreamFrame(github.com/quic-go/quic-go/internal/protocol.ByteCount, github.com/quic-go/quic-go/internal/protocol.VersionNumber) (github.com/quic-go/quic-go/internal/ackhandler.StreamFrame, bool, bool); github.com/quic-go/quic-go.updateSendWindow(github.com/quic-go/quic-go/internal/protocol.ByteCount) }]).openStream()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/streams_map_outgoing.go:120 +0x3fe
  github.com/quic-go/quic-go.(*outgoingStreamsMap[go.shape.interface { CancelRead(github.com/quic-go/quic-go/internal/qerr.StreamErrorCode); CancelWrite(github.com/quic-go/quic-go/internal/qerr.StreamErrorCode); Close() error; Context() context.Context; Read([]uint8) (int, error); SetDeadline(time.Time) error; SetReadDeadline(time.Time) error; SetWriteDeadline(time.Time) error; StreamID() github.com/quic-go/quic-go/internal/protocol.StreamID; Write([]uint8) (int, error); github.com/quic-go/quic-go.closeForShutdown(error); github.com/quic-go/quic-go.getWindowUpdate() github.com/quic-go/quic-go/internal/protocol.ByteCount; github.com/quic-go/quic-go.handleResetStreamFrame(*github.com/quic-go/quic-go/internal/wire.ResetStreamFrame) error; github.com/quic-go/quic-go.handleStopSendingFrame(*github.com/quic-go/quic-go/internal/wire.StopSendingFrame); github.com/quic-go/quic-go.handleStreamFrame(*github.com/quic-go/quic-go/internal/wire.StreamFrame) error; github.com/quic-go/quic-go.hasData() bool; github.com/quic-go/quic-go.popStreamFrame(github.com/quic-go/quic-go/internal/protocol.ByteCount, github.com/quic-go/quic-go/internal/protocol.VersionNumber) (github.com/quic-go/quic-go/internal/ackhandler.StreamFrame, bool, bool); github.com/quic-go/quic-go.updateSendWindow(github.com/quic-go/quic-go/internal/protocol.ByteCount) }]).OpenStreamSync()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/streams_map_outgoing.go:81 +0x374
  github.com/quic-go/quic-go.(*streamsMap).OpenStreamSync()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/streams_map.go:141 +0x10b
  github.com/quic-go/quic-go.(*connection).OpenStreamSync()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:2263 +0x61
  main.callServer()
      …/main.go:212 +0x6e
  main.main.func2()
      …/main.go:56 +0x8f

Previous write at 0x00c00084ebc0 by goroutine 45:
  github.com/quic-go/quic-go/internal/handshake.(*cryptoSetup).handleTransportParameters()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/internal/handshake/crypto_setup.go:303 +0x64
  github.com/quic-go/quic-go/internal/handshake.(*cryptoSetup).handleEvent()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/internal/handshake/crypto_setup.go:275 +0x191
  github.com/quic-go/quic-go/internal/handshake.(*cryptoSetup).handleMessage()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/internal/handshake/crypto_setup.go:254 +0x17e
  github.com/quic-go/quic-go/internal/handshake.(*cryptoSetup).HandleMessage()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/internal/handshake/crypto_setup.go:242 +0x52
  github.com/quic-go/quic-go.(*cryptoStreamManager).HandleCryptoFrame()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/crypto_stream_manager.go:59 +0x1fd
  github.com/quic-go/quic-go.(*connection).handleCryptoFrame()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:1373 +0x64
  github.com/quic-go/quic-go.(*connection).handleFrame()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:1300 +0xd0
  github.com/quic-go/quic-go.(*connection).handleFrames()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:1266 +0x444
  github.com/quic-go/quic-go.(*connection).handleUnpackedLongHeaderPacket()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:1206 +0xdbe
  github.com/quic-go/quic-go.(*connection).handleLongHeaderPacket()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:979 +0xea4
  github.com/quic-go/quic-go.(*connection).handlePacketImpl()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:860 +0x267
  github.com/quic-go/quic-go.(*connection).run()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/connection.go:562 +0x9e8
  github.com/quic-go/quic-go.(*client).dial.func1()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/client.go:214 +0x70

Goroutine 808 (running) created at:
  main.main()
      …/main.go:54 +0xb6d

Goroutine 45 (running) created at:
  github.com/quic-go/quic-go.(*client).dial()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/client.go:213 +0xa6a
  github.com/quic-go/quic-go.dial()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/client.go:159 +0x527
  github.com/quic-go/quic-go.(*Transport).dial()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/transport.go:195 +0x3b2
  github.com/quic-go/quic-go.DialAddrEarly()
      …/go/pkg/mod/github.com/quic-go/quic-go@v0.41.0/client.go:80 +0x344
  main.dial()
      …/main.go:201 +0xcc
  main.main()
      …/main.go:44 +0xa0a

As far as I can see, the code doesn't break any documented contracts with regards to concurrency issues. I have tried to investigate, how exactly the execution comes to these locations, but my knowledge of the quic-go code base is not enough to propose any sort of fix.

[marten-seemann]

This makes sense. We install the server's updated transport parameters when we process the Encrypted Extensions during the handshake. The race condition occurs because creating a new streams needs to look up the peer's flow control limits.

In the 1-RTT handshake this is not a problem, since streams can only be opened after completion of the handshake. When using 0-RTT, streams can be opened concurrently, leading to the race you're seeing here.

[oliverpool]

As a workaround, Allow0RTT can be disabled:

Edit: the data-race still happens with 0-RTT disabled. The stacktrace is like the one in #4228. Sorry for the unrelated noise.

[marten-seemann]

AFAIK disabling this is recommended: https://blog.trailofbits.com/2019/03/25/what-application-developers-need-to-know-about-tls-early-data-0rtt/

It most definitely is not. 0-RTT is only used for GET requests, and only if the request method is set to http3.MethodGet0RTT.