From 1b970713d09a74350386f3fe4389ad4d09c13d0f Mon Sep 17 00:00:00 2001 From: Deleplace Date: Fri, 17 Nov 2023 12:40:48 +0100 Subject: [PATCH] slices: zero the slice elements discarded by Delete, DeleteFunc, Compact, CompactFunc, Replace. Backport from stdlib: to avoid memory leaks in slices that contain pointers, clear the elements between the new length and the original length. Fixes golang/go#63393 Change-Id: I38bf64b27619d8067f2e95ce3c7952ec95ca55b8 Reviewed-on: https://go-review.googlesource.com/c/exp/+/543335 LUCI-TryBot-Result: Go LUCI Reviewed-by: Eli Bendersky --- slices/slices.go | 44 +++++++++----- slices/slices_test.go | 130 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 14 deletions(-) diff --git a/slices/slices.go b/slices/slices.go index 5e8158bba..46ceac343 100644 --- a/slices/slices.go +++ b/slices/slices.go @@ -209,25 +209,37 @@ func Insert[S ~[]E, E any](s S, i int, v ...E) S { return s } +// clearSlice sets all elements up to the length of s to the zero value of E. +// We may use the builtin clear func instead, and remove clearSlice, when upgrading +// to Go 1.21+. +func clearSlice[S ~[]E, E any](s S) { + var zero E + for i := range s { + s[i] = zero + } +} + // Delete removes the elements s[i:j] from s, returning the modified slice. -// Delete panics if s[i:j] is not a valid slice of s. -// Delete is O(len(s)-j), so if many items must be deleted, it is better to +// Delete panics if j > len(s) or s[i:j] is not a valid slice of s. +// Delete is O(len(s)-i), so if many items must be deleted, it is better to // make a single call deleting them all together than to delete one at a time. -// Delete might not modify the elements s[len(s)-(j-i):len(s)]. If those -// elements contain pointers you might consider zeroing those elements so that -// objects they reference can be garbage collected. +// Delete zeroes the elements s[len(s)-(j-i):len(s)]. func Delete[S ~[]E, E any](s S, i, j int) S { - _ = s[i:j] // bounds check + _ = s[i:j:len(s)] // bounds check - return append(s[:i], s[j:]...) + if i == j { + return s + } + + oldlen := len(s) + s = append(s[:i], s[j:]...) + clearSlice(s[len(s):oldlen]) // zero/nil out the obsolete elements, for GC + return s } // DeleteFunc removes any elements from s for which del returns true, // returning the modified slice. -// When DeleteFunc removes m elements, it might not modify the elements -// s[len(s)-m:len(s)]. If those elements contain pointers you might consider -// zeroing those elements so that objects they reference can be garbage -// collected. +// DeleteFunc zeroes the elements between the new length and the original length. func DeleteFunc[S ~[]E, E any](s S, del func(E) bool) S { i := IndexFunc(s, del) if i == -1 { @@ -240,11 +252,13 @@ func DeleteFunc[S ~[]E, E any](s S, del func(E) bool) S { i++ } } + clearSlice(s[i:]) // zero/nil out the obsolete elements, for GC return s[:i] } // Replace replaces the elements s[i:j] by the given v, and returns the // modified slice. Replace panics if s[i:j] is not a valid slice of s. +// When len(v) < (j-i), Replace zeroes the elements between the new length and the original length. func Replace[S ~[]E, E any](s S, i, j int, v ...E) S { _ = s[i:j] // verify that i:j is a valid subslice @@ -272,6 +286,7 @@ func Replace[S ~[]E, E any](s S, i, j int, v ...E) S { if i+len(v) != j { copy(r[i+len(v):], s[j:]) } + clearSlice(s[tot:]) // zero/nil out the obsolete elements, for GC return r } @@ -345,9 +360,7 @@ func Clone[S ~[]E, E any](s S) S { // This is like the uniq command found on Unix. // Compact modifies the contents of the slice s and returns the modified slice, // which may have a smaller length. -// When Compact discards m elements in total, it might not modify the elements -// s[len(s)-m:len(s)]. If those elements contain pointers you might consider -// zeroing those elements so that objects they reference can be garbage collected. +// Compact zeroes the elements between the new length and the original length. func Compact[S ~[]E, E comparable](s S) S { if len(s) < 2 { return s @@ -361,11 +374,13 @@ func Compact[S ~[]E, E comparable](s S) S { i++ } } + clearSlice(s[i:]) // zero/nil out the obsolete elements, for GC return s[:i] } // CompactFunc is like [Compact] but uses an equality function to compare elements. // For runs of elements that compare equal, CompactFunc keeps the first one. +// CompactFunc zeroes the elements between the new length and the original length. func CompactFunc[S ~[]E, E any](s S, eq func(E, E) bool) S { if len(s) < 2 { return s @@ -379,6 +394,7 @@ func CompactFunc[S ~[]E, E any](s S, eq func(E, E) bool) S { i++ } } + clearSlice(s[i:]) // zero/nil out the obsolete elements, for GC return s[:i] } diff --git a/slices/slices_test.go b/slices/slices_test.go index 7371bdb88..38bbede7e 100644 --- a/slices/slices_test.go +++ b/slices/slices_test.go @@ -654,6 +654,39 @@ func TestDeletePanics(t *testing.T) { } } +func TestDeleteClearTail(t *testing.T) { + mem := []*int{new(int), new(int), new(int), new(int), new(int), new(int)} + s := mem[0:5] // there is 1 element beyond len(s), within cap(s) + + s = Delete(s, 2, 4) + + if mem[3] != nil || mem[4] != nil { + // Check that potential memory leak is avoided + t.Errorf("Delete: want nil discarded elements, got %v, %v", mem[3], mem[4]) + } + if mem[5] == nil { + t.Errorf("Delete: want unchanged elements beyond original len, got nil") + } +} + +func TestDeleteFuncClearTail(t *testing.T) { + mem := []*int{new(int), new(int), new(int), new(int), new(int), new(int)} + *mem[2], *mem[3] = 42, 42 + s := mem[0:5] // there is 1 element beyond len(s), within cap(s) + + s = DeleteFunc(s, func(i *int) bool { + return i != nil && *i == 42 + }) + + if mem[3] != nil || mem[4] != nil { + // Check that potential memory leak is avoided + t.Errorf("DeleteFunc: want nil discarded elements, got %v, %v", mem[3], mem[4]) + } + if mem[5] == nil { + t.Errorf("DeleteFunc: want unchanged elements beyond original len, got nil") + } +} + func TestClone(t *testing.T) { s1 := []int{1, 2, 3} s2 := Clone(s1) @@ -757,6 +790,53 @@ func TestCompactFunc(t *testing.T) { } } +func TestCompactClearTail(t *testing.T) { + one, two, three, four := 1, 2, 3, 4 + mem := []*int{&one, &one, &two, &two, &three, &four} + s := mem[0:5] // there is 1 element beyond len(s), within cap(s) + copy := Clone(s) + + s = Compact(s) + + if want := []*int{&one, &two, &three}; !Equal(s, want) { + t.Errorf("Compact(%v) = %v, want %v", copy, s, want) + } + + if mem[3] != nil || mem[4] != nil { + // Check that potential memory leak is avoided + t.Errorf("Compact: want nil discarded elements, got %v, %v", mem[3], mem[4]) + } + if mem[5] != &four { + t.Errorf("Compact: want unchanged element beyond original len, got %v", mem[5]) + } +} + +func TestCompactFuncClearTail(t *testing.T) { + a, b, c, d, e, f := 1, 1, 2, 2, 3, 4 + mem := []*int{&a, &b, &c, &d, &e, &f} + s := mem[0:5] // there is 1 element beyond len(s), within cap(s) + copy := Clone(s) + + s = CompactFunc(s, func(x, y *int) bool { + if x == nil || y == nil { + return x == y + } + return *x == *y + }) + + if want := []*int{&a, &c, &e}; !Equal(s, want) { + t.Errorf("CompactFunc(%v) = %v, want %v", copy, s, want) + } + + if mem[3] != nil || mem[4] != nil { + // Check that potential memory leak is avoided + t.Errorf("CompactFunc: want nil discarded elements, got %v, %v", mem[3], mem[4]) + } + if mem[5] != &f { + t.Errorf("CompactFunc: want unchanged elements beyond original len, got %v", mem[5]) + } +} + func BenchmarkCompactFunc_Large(b *testing.B) { type Large [4 * 1024]byte @@ -922,6 +1002,56 @@ func TestReplacePanics(t *testing.T) { } } +func TestReplaceGrow(t *testing.T) { + // When Replace needs to allocate a new slice, we want the original slice + // to not be changed. + a, b, c, d, e, f := 1, 2, 3, 4, 5, 6 + mem := []*int{&a, &b, &c, &d, &e, &f} + memcopy := Clone(mem) + s := mem[0:5] // there is 1 element beyond len(s), within cap(s) + copy := Clone(s) + original := s + + // The new elements don't fit within cap(s), so Replace will allocate. + z := 99 + s = Replace(s, 1, 3, &z, &z, &z, &z) + + if want := []*int{&a, &z, &z, &z, &z, &d, &e}; !Equal(s, want) { + t.Errorf("Replace(%v, 1, 3, %v, %v, %v, %v) = %v, want %v", copy, &z, &z, &z, &z, s, want) + } + + if !Equal(original, copy) { + t.Errorf("original slice has changed, got %v, want %v", original, copy) + } + + if !Equal(mem, memcopy) { + // Changing the original tail s[len(s):cap(s)] is unwanted + t.Errorf("original backing memory has changed, got %v, want %v", mem, memcopy) + } +} + +func TestReplaceClearTail(t *testing.T) { + a, b, c, d, e, f := 1, 2, 3, 4, 5, 6 + mem := []*int{&a, &b, &c, &d, &e, &f} + s := mem[0:5] // there is 1 element beyond len(s), within cap(s) + copy := Clone(s) + + y, z := 8, 9 + s = Replace(s, 1, 4, &y, &z) + + if want := []*int{&a, &y, &z, &e}; !Equal(s, want) { + t.Errorf("Replace(%v) = %v, want %v", copy, s, want) + } + + if mem[4] != nil { + // Check that potential memory leak is avoided + t.Errorf("Replace: want nil discarded element, got %v", mem[4]) + } + if mem[5] != &f { + t.Errorf("Replace: want unchanged elements beyond original len, got %v", mem[5]) + } +} + func TestReplaceOverlap(t *testing.T) { const N = 10 a := make([]int, N)