wgengine/magicsock: fix panic when rebinding fails #6650
Conversation
|
(open to reviewer suggestions while Brad is out) |
bradfitz
left a comment
There was a problem hiding this comment.
TV's done and waiting for Advent of Code before sleeping, so surprise review! 😂
wgengine/magicsock/magicsock.go
Outdated
| // pconn isn't really needed, but makes some of the code simpler | ||
| // to keep it in a type safe form. |
There was a problem hiding this comment.
this comment is old
while you're here, document whether this atomic.Pointer can ever be nil and if so, what that means (it seems like it's never nil)
also, clarify that it's not a pointer to the (*RebindingUDPConn).pconn value
might be worth adding a little test that just calls setConnLocked to two different PacketConn impl types lest anybody "optimize" this code in the future and revert this back to a syncs.AtomicValue.
There was a problem hiding this comment.
Now that I've ready the comment a bit more carefully, it says:
// pconn isn't really needed, but makes some of the code simpler
// to keep it in a type safe form.
But pconnAtomic is typesafe now, there used to be a TODO here that was removed in a9f6cd4#diff-7b44a473ce1a6ac00f78933d1f0651f64f7bd20de479235a324d108c3998cbc7L2982.
I still think we want pconnAtomic vs. pconn (to handle cases where the connection is rebound during a read/write), did my best to reword this.
Added a test.
| func (c *RebindingUDPConn) ReadFrom(b []byte) (int, net.Addr, error) { | ||
| for { | ||
| pconn := c.pconnAtomic.Load() | ||
| pconn := *c.pconnAtomic.Load() |
There was a problem hiding this comment.
Is c.pconnAtomic gauranteed to be stored prior to this call? Otherwise, this could become a nil-pointer panic now.
There was a problem hiding this comment.
Based on my reading (where we call rebind in NewConn) it should always be stored. Though even in the old code, if there was no store there would have been a nil-pointer panic on the next line, when calling pconn.ReadFrom
We would replace the existing real implementation of nettype.PacketConn with a blockForeverConn, but that violates the contract of atomic.Value (where the type cannot change). Fix by switching to a pointer value (atomic.Pointer[nettype.PacketConn]). A longstanding issue, but became more prevalent when we started binding connections to interfaces on macOS and iOS (#6566), which could lead to the bind call failing if the interface was no longer available. Fixes #6641 Signed-off-by: Mihai Parparita <mihai@tailscale.com>
502cdc8 to
56d7bb4
Compare
We would replace the existing real implementation of
nettype.PacketConnwith ablockForeverConn, but that violates the contract ofatomic.Value (where the type cannot change). Fix by switching to a pointer value (atomic.Pointer[nettype.PacketConn]).A longstanding issue, but became more prevalent when we started binding connections to interfaces on macOS and iOS (#6566), which could lead to the bind call failing if the interface was no longer available.
Fixes #6641