GSO handling causes data race about quic-go HOT 5 OPEN

Thanks for reporting. Changing the type of gotGSOError to atomic.Bool is probably not the correct solution though, as it just masks the underlying race condition.

from quic-go.

Actual data race happens when accessing sconn.gotGSOError. Changing its' type to atomic.Bool solves the problem. I suspect wroteFirstPacket field may cause the same issue as it can also be edited by different goroutines simultaniously. But I did not dig deep into more details whether doing that would be a good enough solution. Please let me know if it would and I could create a PR

from quic-go.

Affected as well (I initially thought it was a data race like #4303 - however even when disabling 0-RTT, the data race appears sometime).

as it just masks the underlying race condition

I have run it a couple of times with a simple test of mine and apparently the GSOError does not appear on the first write (in the following example it appears on the 7th):

> 1x: gotGSOError false (send_conn.go:97)
> 6x: writePacket error nil (send_conn.go:63)
> 2x: gotGSOError false (send_conn.go:97)
> 1x: gotGSOError false (send_conn.go:97)
> 1x: writePacket error(*net.OpError)  (send_conn.go:63) <- this is a GSOError

With such an execution trace, I see two possible solutions:

• make c.gotGSOError atomic or mutex protected
• adjust client.go:467 connState := conn.ConnectionState().TLS since this triggers the data race, but does not care about the GSO capability (it access the variable within ConnectionState(), but only takes the TLS field). For instance the ConnectionState method could be split:

type Connection interface {
	// ...

	// ConnectionState returns information about the TLS connection state
	ConnectionState() tls.ConnectionState
	// QuicState returns basic details about the QUIC connection.
	// Warning: This API should not be considered stable and might change soon.
	QuicState() QuicState

	// ...
}   

I would be willing to draft a PR to address this.

from quic-go.

as it just masks the underlying race condition

I have run it a couple of times with a simple test of mine and apparently the GSOError does not appear on the first write (in the following example it appears on the 7th):

Interesting! What system are you running on?

• adjust client.go:467 connState := conn.ConnectionState().TLS since this triggers the data race, but does not care about the GSO capability (it access the variable within ConnectionState(), but only takes the TLS field). For instance the ConnectionState method could be split:

This wouldn't solve the race, it would just hide it, right?

from quic-go.

What system are you running on?

Arch Linux.

Investigating a bit more, the GSOError happens at the first GSO packet (which is the 7th). The first 6 packets have gsoSize set to 0 (during which handshakeConfirmed is false).

adjust client.go:467 connState := conn.ConnectionState().TLS since this triggers the data race, but does not care about the GSO capability (it access the variable within ConnectionState(), but only takes the TLS field). For instance the ConnectionState method could be split:

This wouldn't solve the race, it would just hide it, right?

I think this would solve the race because it happens during the dial phase, before the connection is returned to the caller.

So nothing outside of the library should be able to call ConnectionState before the GSOError value is settled (except if you want to handle GSO being disabled during a connection, in which case I don't see any solution without a mutex).

from quic-go.