-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
potential failure on Windows if UDP payload > 1472 bytes #3956
Comments
I'm not sure how the tests are set up, but from the naming I infer that the connection is already set up before the test happens. For this problem, the packets have to arrive while Accept() is waiting for new connections. |
Why’s that? They all go through the same listen loop. |
Oh, I see. I didn't realize that was the same listen loop. I tried running your test case, and it does fail for me on Windows 10 Home. I'll attach the output of |
I wonder if your Windows installation might have a lower MTU set on the loopback interface? Maybe the packet is getting truncated or fragmented before it reaches the read side? |
I didn't realize we're not running our integration tests on Windows in CI 🤦🏻♂️ Sorry for that! I enabled them on Windows, and sure enough, they fail now as well: https://github.com/quic-go/quic-go/actions/runs/5603406031/jobs/10249977347. |
I've found at least one place where
Temporary()
isn't doing the right thing now:If you send a UDP packet with a payload of 1472 bytes to a quic-go listener (where
listener.Accept()
is being called) on Windows, the packetconn read call returnswindows.WSAEMSGSIZE
, which is apparently not.Temporary()
. The Accept call fails, and It would seem that a lot of quic-go-using applications panic at this point.A payload larger than
MaxPacketBufferSize
=1452 bytes is only possible when using IPv4 (or probably with ethernet jumbo frames, too), because IPv4 headers are smaller than IPv6 headers, and the math involved in coming up withMaxPacketBufferSize
apparently assumes all packets are IPv6? I would argue thatMaxPacketBufferSize
is in fact wrong for this reason, but this is beside the point.On Linux and other unix-y OSes,
recvfrom
doesn't return an error when the incoming packet is too large, so there's no problem there.One way to solve the problem on Windows would be by adding a few lines to
(*Transport).listen()
after checkingnerr.Temporary()
:But I imagine you might not want to get the function overly cluttered up with special checks like this, so I don't know if that's the "right" solution.
Originally posted by @thepaul in #1737 (comment)
The text was updated successfully, but these errors were encountered: