Fix the buffer overlap panic when read a udp/packet socket #97
Conversation
shadowstream/packet.go
Outdated
| @@ -46,19 +46,20 @@ func Unpack(dst, pkt []byte, s Cipher) ([]byte, error) { | |||
| type packetConn struct { | |||
| net.PacketConn | |||
| Cipher | |||
| buf []byte | |||
| wbuf []byte | |||
| rbuf []byte | |||
shadowstream/packet.go
Outdated
| @@ -67,10 +68,10 @@ func (c *packetConn) WriteTo(b []byte, addr net.Addr) (int, error) { | |||
| } | |||
|
|
|||
| func (c *packetConn) ReadFrom(b []byte) (int, net.Addr, error) { | |||
| n, addr, err := c.PacketConn.ReadFrom(b) | |||
| n, addr, err := c.PacketConn.ReadFrom(c.rbuf) | |||
shadowstream/packet.go
Outdated
| if err != nil { | ||
| return n, addr, err | ||
| } | ||
| b, err = Unpack(b, b[:n], c.Cipher) | ||
| b, err = Unpack(b, c.rbuf[:n], c.Cipher) |
There was a problem hiding this comment.
…and memmove here
bb, err = Unpack(b[c.IVSize():], b[:n], c. Cipher)
if err != nil {
return n, addr, err
}
copy(b, bb)
return len(bb), addr, errThis way, we save 64KB per UDP connection and never have to worry about locking.
There was a problem hiding this comment.
the Unpack will still panic, since b[c.IVSize():] and b[:n] partially overlaps.
There was a problem hiding this comment.
How? Only XORKeyStream forbids inexact overlap. Unpack does not care.
|
We don't really need 64k buffer for read, since the MTU is much smaller, it xpuld be 10k or less. And if we use the buffer b in place, we have to call copy(). I don't think copy is better than just using a new buffer. |
|
Max UDP packet size is a little less than 64KB. We cannot assume it'll be smaller due to potential fragmentation reassembly. And the packet might not come from WAN at all, in which case 1500 MTU cannot be assumed either. As for the extra copy, Golang implements it via memmove which is very efficient (especially when the packet size is small and it fits in the cache). |
|
Fixed as you wish. By the way, I have a local build which is compatible with Shadowsocks-android as its built-in client. But it uses some features only available after go 1.11. If you would like to keep the code here, I can make a PR after go 1.11 is released. |
|
Thanks for the PR! BTW do we need to fix the AEAD packet Unpack as well? I think it will also be affected by the Go 1.11 InexactOverlap constraint. What features do you use from Go 1.11? I'd like to take a look :D |
|
you are right, the AEAD part also needs to be fixed. New APIs in the net package have been exposed in go 1.11, I need to protect the sockets which are connecting the server from VPN connections, before calling equivalent |
…erlap_fix Fix the buffer overlap panic when read a udp/packet socket
As explained in #95
It should be safe without locking when reading from the packet socket.