From 737124ef70e54f3aa2e4f75114742f757d58aab4 Mon Sep 17 00:00:00 2001
From: Dmytro Shynkevych <dm.shynk@gmail.com>
Date: Tue, 26 May 2020 18:14:19 -0400
Subject: [PATCH] tstun: tolerate zero reads

Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
---
 wgengine/tstun/tun.go | 48 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/wgengine/tstun/tun.go b/wgengine/tstun/tun.go
index e6202e8c5..c798fc37b 100644
--- a/wgengine/tstun/tun.go
+++ b/wgengine/tstun/tun.go
@@ -29,11 +29,14 @@ const (
 const MaxPacketSize = device.MaxContentSize
 
 var (
-	ErrClosed       = errors.New("device closed")
-	ErrFiltered     = errors.New("packet dropped by filter")
-	ErrPacketTooBig = errors.New("packet too big")
+	// ErrClosed is returned when attempting an operation on a closed TUN.
+	ErrClosed = errors.New("device closed")
+	// ErrFiltered is returned when the acted-on packet is rejected by a filter.
+	ErrFiltered = errors.New("packet dropped by filter")
 )
 
+var errPacketTooBig = errors.New("packet too big")
+
 // TUN wraps a tun.Device from wireguard-go,
 // augmenting it with filtering and packet injection.
 // All the added work happens in Read and Write:
@@ -54,11 +57,15 @@ type TUN struct {
 	// errors is the error queue populated by poll.
 	errors chan error
 	// outbound is the queue by which packets leave the TUN device.
+	//
 	// The directions are relative to the network, not the device:
 	// inbound packets arrive via UDP and are written into the TUN device;
 	// outbound packets are read from the TUN device and sent out via UDP.
 	// This queue is needed because although inbound writes are synchronous,
 	// the other direction must wait on a Wireguard goroutine to poll it.
+	//
+	// Empty reads are skipped by Wireguard, so it is always legal
+	// to discard an empty packet instead of sending it through t.outbound.
 	outbound chan []byte
 
 	// fitler stores the currently active package filter
@@ -142,13 +149,21 @@ func (t *TUN) poll() {
 				// In principle, read errors are not fatal (but wireguard-go disagrees).
 				t.bufferConsumed <- struct{}{}
 			}
-		} else {
-			select {
-			case <-t.closed:
-				return
-			case t.outbound <- t.buffer[readOffset : readOffset+n]:
-				// continue
-			}
+			continue
+		}
+
+		// Wireguard will skip an empty read,
+		// so we might as well do it here to avoid the send through t.outbound.
+		if n == 0 {
+			t.bufferConsumed <- struct{}{}
+			continue
+		}
+
+		select {
+		case <-t.closed:
+			return
+		case t.outbound <- t.buffer[readOffset : readOffset+n]:
+			// continue
 		}
 	}
 }
@@ -180,6 +195,7 @@ func (t *TUN) Read(buf []byte, offset int) (int, error) {
 		n = copy(buf[offset:], packet)
 		// t.buffer has a fixed location in memory,
 		// so this is the easiest way to tell when it has been consumed.
+		// &packet[0] can be used because empty packets do not reach t.outbound.
 		if &packet[0] == &t.buffer[readOffset] {
 			t.bufferConsumed <- struct{}{}
 		}
@@ -240,9 +256,13 @@ func (t *TUN) SetFilter(filt *filter.Filter) {
 // InjectInbound makes the TUN device behave as if a packet
 // with the given contents was received from the network.
 // It blocks and does not take ownership of the packet.
+// Injecting an empty packet is a no-op.
 func (t *TUN) InjectInbound(packet []byte) error {
 	if len(packet) > MaxPacketSize {
-		return ErrPacketTooBig
+		return errPacketTooBig
+	}
+	if len(packet) == 0 {
+		return nil
 	}
 	_, err := t.Write(packet, 0)
 	return err
@@ -251,9 +271,13 @@ func (t *TUN) InjectInbound(packet []byte) error {
 // InjectOutbound makes the TUN device behave as if a packet
 // with the given contents was sent to the network.
 // It does not block, but takes ownership of the packet.
+// Injecting an empty packet is a no-op.
 func (t *TUN) InjectOutbound(packet []byte) error {
 	if len(packet) > MaxPacketSize {
-		return ErrPacketTooBig
+		return errPacketTooBig
+	}
+	if len(packet) == 0 {
+		return nil
 	}
 	select {
 	case <-t.closed: