diff options
author | David Fifield <david@bamsoftware.com> | 2023-03-13 12:57:35 -0600 |
---|---|---|
committer | David Fifield <david@bamsoftware.com> | 2023-03-13 12:57:35 -0600 |
commit | d2858aeb7ec50ae09b9a7e2e2a910ae31cec62bd (patch) | |
tree | 75b04949f1e895a215d19ef573803cc790be762c | |
parent | b63d2272bfd262f5166c44f8f88330ed7a732009 (diff) | |
download | snowflake-d2858aeb7ec50ae09b9a7e2e2a910ae31cec62bd.tar.gz snowflake-d2858aeb7ec50ae09b9a7e2e2a910ae31cec62bd.zip |
Revert "Take ownership of buffer in QueuePacketConn QueueIncoming/WriteTo."
This reverts commit 839d2218837dfbd1682ff39b375f45660b3974b5. (Except for
the added benchmarks in queuepacketconn_test.go.) This change
corresponds to the issues #40187 and #40199.
The analysis in https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/issues/40199
was wrong; kcp-go does reuse the buffers it passes to
QueuePacketConn.WriteTo. This led to unsynchronized reuse of packet
buffers and mangled packets observable at the client:
https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/issues/40260.
Undoing the change in QueuePacketConn.QueueIncoming as well, for
symmetry, even though it is not implicated in any correctness problems.
-rw-r--r-- | common/turbotunnel/queuepacketconn.go | 22 |
1 files changed, 13 insertions, 9 deletions
diff --git a/common/turbotunnel/queuepacketconn.go b/common/turbotunnel/queuepacketconn.go index e11f8d8..14a9833 100644 --- a/common/turbotunnel/queuepacketconn.go +++ b/common/turbotunnel/queuepacketconn.go @@ -42,9 +42,8 @@ func NewQueuePacketConn(localAddr net.Addr, timeout time.Duration) *QueuePacketC } } -// QueueIncoming queues an incoming packet and its source address, to be -// returned in a future call to ReadFrom. This function takes ownership of p and -// the caller must not reuse it. +// QueueIncoming queues and incoming packet and its source address, to be +// returned in a future call to ReadFrom. func (c *QueuePacketConn) QueueIncoming(p []byte, addr net.Addr) { select { case <-c.closed: @@ -52,8 +51,11 @@ func (c *QueuePacketConn) QueueIncoming(p []byte, addr net.Addr) { return default: } + // Copy the slice so that the caller may reuse it. + buf := make([]byte, len(p)) + copy(buf, p) select { - case c.recvQueue <- taggedPacket{p, addr}: + case c.recvQueue <- taggedPacket{buf, addr}: default: // Drop the incoming packet if the receive queue is full. } @@ -82,20 +84,22 @@ func (c *QueuePacketConn) ReadFrom(p []byte) (int, net.Addr, error) { } // WriteTo queues an outgoing packet for the given address. The queue can later -// be retrieved using the OutgoingQueue method. This function takes ownership of -// p and the caller must not reuse it. +// be retrieved using the OutgoingQueue method. func (c *QueuePacketConn) WriteTo(p []byte, addr net.Addr) (int, error) { select { case <-c.closed: return 0, &net.OpError{Op: "write", Net: c.LocalAddr().Network(), Addr: c.LocalAddr(), Err: c.err.Load().(error)} default: } + // Copy the slice so that the caller may reuse it. + buf := make([]byte, len(p)) + copy(buf, p) select { - case c.clients.SendQueue(addr) <- p: - return len(p), nil + case c.clients.SendQueue(addr) <- buf: + return len(buf), nil default: // Drop the outgoing packet if the send queue is full. - return len(p), nil + return len(buf), nil } } |