Always pass buffer.VectorisedView by value

PiperOrigin-RevId: 212757571
Change-Id: I04200df9e45c21eb64951cd2802532fa84afcb1a

5 files changed, 50 insertions, 64 deletions

diff --git a/pkg/tcpip/network/fragmentation/frag_heap.go b/pkg/tcpip/network/fragmentation/frag_heap.go
index 073882e99..6c7faafe4 100644
--- a/pkg/tcpip/network/fragmentation/frag_heap.go
+++ b/pkg/tcpip/network/fragmentation/frag_heap.go
@@ -23,7 +23,7 @@ import (
 
 type fragment struct {
 	offset uint16
-	vv     *buffer.VectorisedView
+	vv     buffer.VectorisedView
 }
 
 type fragHeap []fragment
@@ -60,7 +60,7 @@ func (h *fragHeap) reassemble() (buffer.VectorisedView, error) {
 	size := curr.vv.Size()
 
 	if curr.offset != 0 {
-		return buffer.NewVectorisedView(0, nil), fmt.Errorf("offset of the first packet is != 0 (%d)", curr.offset)
+		return buffer.VectorisedView{}, fmt.Errorf("offset of the first packet is != 0 (%d)", curr.offset)
 	}
 
 	for h.Len() > 0 {
@@ -68,7 +68,7 @@ func (h *fragHeap) reassemble() (buffer.VectorisedView, error) {
 		if int(curr.offset) < size {
 			curr.vv.TrimFront(size - int(curr.offset))
 		} else if int(curr.offset) > size {
-			return buffer.NewVectorisedView(0, nil), fmt.Errorf("packet has a hole, expected offset %d, got %d", size, curr.offset)
+			return buffer.VectorisedView{}, fmt.Errorf("packet has a hole, expected offset %d, got %d", size, curr.offset)
 		}
 		size += curr.vv.Size()
 		views = append(views, curr.vv.Views()...)
diff --git a/pkg/tcpip/network/fragmentation/frag_heap_test.go b/pkg/tcpip/network/fragmentation/frag_heap_test.go
index a2fe80264..a15540634 100644
--- a/pkg/tcpip/network/fragmentation/frag_heap_test.go
+++ b/pkg/tcpip/network/fragmentation/frag_heap_test.go
@@ -25,7 +25,7 @@ import (
 var reassambleTestCases = []struct {
 	comment string
 	in      []fragment
-	want    *buffer.VectorisedView
+	want    buffer.VectorisedView
 }{
 	{
 		comment: "Non-overlapping in-order",
@@ -87,21 +87,25 @@ var reassambleTestCases = []struct {
 
 func TestReassamble(t *testing.T) {
 	for _, c := range reassambleTestCases {
-		h := (fragHeap)(make([]fragment, 0, 8))
-		heap.Init(&h)
-		for _, f := range c.in {
-			heap.Push(&h, f)
-		}
-		got, _ := h.reassemble()
-
-		if !reflect.DeepEqual(got, *c.want) {
-			t.Errorf("Test \"%s\" reassembling failed. Got %v. Want %v", c.comment, got, *c.want)
-		}
+		t.Run(c.comment, func(t *testing.T) {
+			h := make(fragHeap, 0, 8)
+			heap.Init(&h)
+			for _, f := range c.in {
+				heap.Push(&h, f)
+			}
+			got, err := h.reassemble()
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !reflect.DeepEqual(got, c.want) {
+				t.Errorf("got reassemble(%+v) = %v, want = %v", c.in, got, c.want)
+			}
+		})
 	}
 }
 
 func TestReassambleFailsForNonZeroOffset(t *testing.T) {
-	h := (fragHeap)(make([]fragment, 0, 8))
+	h := make(fragHeap, 0, 8)
 	heap.Init(&h)
 	heap.Push(&h, fragment{offset: 1, vv: vv(1, "0")})
 	_, err := h.reassemble()
@@ -111,7 +115,7 @@ func TestReassambleFailsForNonZeroOffset(t *testing.T) {
 }
 
 func TestReassambleFailsForHoles(t *testing.T) {
-	h := (fragHeap)(make([]fragment, 0, 8))
+	h := make(fragHeap, 0, 8)
 	heap.Init(&h)
 	heap.Push(&h, fragment{offset: 0, vv: vv(1, "0")})
 	heap.Push(&h, fragment{offset: 2, vv: vv(1, "1")})
diff --git a/pkg/tcpip/network/fragmentation/fragmentation.go b/pkg/tcpip/network/fragmentation/fragmentation.go
index 21497f876..885e3cca2 100644
--- a/pkg/tcpip/network/fragmentation/fragmentation.go
+++ b/pkg/tcpip/network/fragmentation/fragmentation.go
@@ -82,7 +82,7 @@ func NewFragmentation(highMemoryLimit, lowMemoryLimit int, reassemblingTimeout t
 
 // Process processes an incoming fragment beloning to an ID
 // and returns a complete packet when all the packets belonging to that ID have been received.
-func (f *Fragmentation) Process(id uint32, first, last uint16, more bool, vv *buffer.VectorisedView) (buffer.VectorisedView, bool) {
+func (f *Fragmentation) Process(id uint32, first, last uint16, more bool, vv buffer.VectorisedView) (buffer.VectorisedView, bool) {
 	f.mu.Lock()
 	r, ok := f.reassemblers[id]
 	if ok && r.tooOld(f.timeout) {
diff --git a/pkg/tcpip/network/fragmentation/fragmentation_test.go b/pkg/tcpip/network/fragmentation/fragmentation_test.go
index 7320e594f..fc62a15dd 100644
--- a/pkg/tcpip/network/fragmentation/fragmentation_test.go
+++ b/pkg/tcpip/network/fragmentation/fragmentation_test.go
@@ -23,19 +23,13 @@ import (
 )
 
 // vv is a helper to build VectorisedView from different strings.
-func vv(size int, pieces ...string) *buffer.VectorisedView {
+func vv(size int, pieces ...string) buffer.VectorisedView {
 	views := make([]buffer.View, len(pieces))
 	for i, p := range pieces {
 		views[i] = []byte(p)
 	}
 
-	vv := buffer.NewVectorisedView(size, views)
-	return &vv
-}
-
-func emptyVv() *buffer.VectorisedView {
-	vv := buffer.NewVectorisedView(0, nil)
-	return &vv
+	return buffer.NewVectorisedView(size, views)
 }
 
 type processInput struct {
@@ -43,11 +37,11 @@ type processInput struct {
 	first uint16
 	last  uint16
 	more  bool
-	vv    *buffer.VectorisedView
+	vv    buffer.VectorisedView
 }
 
 type processOutput struct {
-	vv   *buffer.VectorisedView
+	vv   buffer.VectorisedView
 	done bool
 }
 
@@ -63,7 +57,7 @@ var processTestCases = []struct {
 			{id: 0, first: 2, last: 3, more: false, vv: vv(2, "23")},
 		},
 		out: []processOutput{
-			{vv: emptyVv(), done: false},
+			{vv: buffer.VectorisedView{}, done: false},
 			{vv: vv(4, "01", "23"), done: true},
 		},
 	},
@@ -76,8 +70,8 @@ var processTestCases = []struct {
 			{id: 0, first: 2, last: 3, more: false, vv: vv(2, "23")},
 		},
 		out: []processOutput{
-			{vv: emptyVv(), done: false},
-			{vv: emptyVv(), done: false},
+			{vv: buffer.VectorisedView{}, done: false},
+			{vv: buffer.VectorisedView{}, done: false},
 			{vv: vv(4, "ab", "cd"), done: true},
 			{vv: vv(4, "01", "23"), done: true},
 		},
@@ -86,26 +80,28 @@ var processTestCases = []struct {
 
 func TestFragmentationProcess(t *testing.T) {
 	for _, c := range processTestCases {
-		f := NewFragmentation(1024, 512, DefaultReassembleTimeout)
-		for i, in := range c.in {
-			vv, done := f.Process(in.id, in.first, in.last, in.more, in.vv)
-			if !reflect.DeepEqual(vv, *(c.out[i].vv)) {
-				t.Errorf("Test \"%s\" Process() returned a wrong vv. Got %v. Want %v", c.comment, vv, *(c.out[i].vv))
-			}
-			if done != c.out[i].done {
-				t.Errorf("Test \"%s\" Process() returned a wrong done. Got %t. Want %t", c.comment, done, c.out[i].done)
-			}
-			if c.out[i].done {
-				if _, ok := f.reassemblers[in.id]; ok {
-					t.Errorf("Test \"%s\" Process() didn't remove buffer from reassemblers.", c.comment)
+		t.Run(c.comment, func(t *testing.T) {
+			f := NewFragmentation(1024, 512, DefaultReassembleTimeout)
+			for i, in := range c.in {
+				vv, done := f.Process(in.id, in.first, in.last, in.more, in.vv)
+				if !reflect.DeepEqual(vv, c.out[i].vv) {
+					t.Errorf("got Process(%d) = %+v, want = %+v", i, vv, c.out[i].vv)
+				}
+				if done != c.out[i].done {
+					t.Errorf("got Process(%d) = %+v, want = %+v", i, done, c.out[i].done)
 				}
-				for n := f.rList.Front(); n != nil; n = n.Next() {
-					if n.id == in.id {
-						t.Errorf("Test \"%s\" Process() didn't remove buffer from rList.", c.comment)
+				if c.out[i].done {
+					if _, ok := f.reassemblers[in.id]; ok {
+						t.Errorf("Process(%d) did not remove buffer from reassemblers", i)
+					}
+					for n := f.rList.Front(); n != nil; n = n.Next() {
+						if n.id == in.id {
+							t.Errorf("Process(%d) did not remove buffer from rList", i)
+						}
 					}
 				}
 			}
-		}
+		})
 	}
 }
 
@@ -161,16 +157,3 @@ func TestMemoryLimitsIgnoresDuplicates(t *testing.T) {
 		t.Errorf("Wrong size, duplicates are not handled correctly: got=%d, want=%d.", got, want)
 	}
 }
-
-func TestFragmentationViewsDoNotEscape(t *testing.T) {
-	f := NewFragmentation(1024, 512, DefaultReassembleTimeout)
-	in := vv(2, "0", "1")
-	f.Process(0, 0, 1, true, in)
-	// Modify input view.
-	in.RemoveFirst()
-	got, _ := f.Process(0, 2, 2, false, vv(1, "2"))
-	want := vv(3, "0", "1", "2")
-	if !reflect.DeepEqual(got, *want) {
-		t.Errorf("Process() returned a wrong vv. Got %v. Want %v", got, *want)
-	}
-}
diff --git a/pkg/tcpip/network/fragmentation/reassembler.go b/pkg/tcpip/network/fragmentation/reassembler.go
index 7c465c1ac..b57fe82ec 100644
--- a/pkg/tcpip/network/fragmentation/reassembler.go
+++ b/pkg/tcpip/network/fragmentation/reassembler.go
@@ -78,7 +78,7 @@ func (r *reassembler) updateHoles(first, last uint16, more bool) bool {
 	return used
 }
 
-func (r *reassembler) process(first, last uint16, more bool, vv *buffer.VectorisedView) (buffer.VectorisedView, bool, int) {
+func (r *reassembler) process(first, last uint16, more bool, vv buffer.VectorisedView) (buffer.VectorisedView, bool, int) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 	consumed := 0
@@ -86,18 +86,17 @@ func (r *reassembler) process(first, last uint16, more bool, vv *buffer.Vectoris
 		// A concurrent goroutine might have already reassembled
 		// the packet and emptied the heap while this goroutine
 		// was waiting on the mutex. We don't have to do anything in this case.
-		return buffer.NewVectorisedView(0, nil), false, consumed
+		return buffer.VectorisedView{}, false, consumed
 	}
 	if r.updateHoles(first, last, more) {
 		// We store the incoming packet only if it filled some holes.
-		uu := vv.Clone(nil)
-		heap.Push(&r.heap, fragment{offset: first, vv: &uu})
+		heap.Push(&r.heap, fragment{offset: first, vv: vv.Clone(nil)})
 		consumed = vv.Size()
 		r.size += consumed
 	}
 	// Check if all the holes have been deleted and we are ready to reassamble.
 	if r.deleted < len(r.holes) {
-		return buffer.NewVectorisedView(0, nil), false, consumed
+		return buffer.VectorisedView{}, false, consumed
 	}
 	res, err := r.heap.reassemble()
 	if err != nil {