diff options
author | Bhasker Hariharan <bhaskerh@google.com> | 2021-07-08 11:32:37 -0700 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2021-07-08 11:35:18 -0700 |
commit | 1fc7a9eac2f27053ed4ca138c6568b14ef520648 (patch) | |
tree | 64b27d492dc851dafc97841eaace6159a4eae1a0 | |
parent | 02fec8dba5a6148e44da2715e26246a5d91e64fa (diff) |
Do not queue zero sized segments.
Commit 16b751b6c610ec2c5a913cb8a818e9239ee7da71 introduced a bug where writes of
zero size would end up queueing a zero sized segment which will cause the
sandbox to panic when trying to send a zero sized segment(e.g. after an RTO) as
netstack asserts that the all non FIN segments have size > 0.
This change adds the check for a zero sized payload back to avoid queueing
such segments. The associated test panics without the fix and passes with it.
PiperOrigin-RevId: 383677884
-rw-r--r-- | pkg/tcpip/transport/tcp/endpoint.go | 6 | ||||
-rw-r--r-- | pkg/tcpip/transport/tcp/tcp_test.go | 32 |
2 files changed, 38 insertions, 0 deletions
diff --git a/pkg/tcpip/transport/tcp/endpoint.go b/pkg/tcpip/transport/tcp/endpoint.go index 1ed4ba419..9945fdd6b 100644 --- a/pkg/tcpip/transport/tcp/endpoint.go +++ b/pkg/tcpip/transport/tcp/endpoint.go @@ -1530,6 +1530,12 @@ func (e *endpoint) queueSegment(p tcpip.Payloader, opts tcpip.WriteOptions) (*se if err != nil { return nil, 0, err } + + // Do not queue zero length segments. + if len(v) == 0 { + return nil, 0, nil + } + if !opts.Atomic { // Since we released locks in between it's possible that the // endpoint transitioned to a CLOSED/ERROR states so make diff --git a/pkg/tcpip/transport/tcp/tcp_test.go b/pkg/tcpip/transport/tcp/tcp_test.go index 71c4aa85d..3f67a1f1c 100644 --- a/pkg/tcpip/transport/tcp/tcp_test.go +++ b/pkg/tcpip/transport/tcp/tcp_test.go @@ -3624,6 +3624,38 @@ func TestMaxRTO(t *testing.T) { } } +// TestZeroSizedWriteRetransmit tests that a zero sized write should not +// result in a panic on an RTO as no segment should have been queued for +// a zero sized write. +func TestZeroSizedWriteRetransmit(t *testing.T) { + c := context.New(t, defaultMTU) + defer c.Cleanup() + + c.CreateConnected(context.TestInitialSequenceNumber, 30000 /* rcvWnd */, -1 /* epRcvBuf */) + + var r bytes.Reader + _, err := c.EP.Write(&r, tcpip.WriteOptions{}) + if err != nil { + t.Fatalf("Write failed: %s", err) + } + // Now do a non-zero sized write to trigger actual sending of data. + r.Reset(make([]byte, 1)) + _, err = c.EP.Write(&r, tcpip.WriteOptions{}) + if err != nil { + t.Fatalf("Write failed: %s", err) + } + // Do not ACK the packet and expect an original transmit and a + // retransmit. This should not cause a panic. + for i := 0; i < 2; i++ { + checker.IPv4(t, c.GetPacket(), + checker.TCP( + checker.DstPort(context.TestPort), + checker.TCPFlagsMatch(header.TCPFlagAck, ^header.TCPFlagPsh), + ), + ) + } +} + // TestRetransmitIPv4IDUniqueness tests that the IPv4 Identification field is // unique on retransmits. func TestRetransmitIPv4IDUniqueness(t *testing.T) { |