slices: Clone keeps source array alive when source slice is zero length

[diegommm]

Go version

go version go1.22.5 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/gopher/.cache/go-build'
GOENV='/home/gopher/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/gopher/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/gopher/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/snap/go/10660'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/snap/go/10660/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3287061668=/tmp/go-build -gno-record-gcc-switches'

What did you do?

See this playground.

package main

import (
	"fmt"
	"runtime"
	"slices"
)

func main() {
	fmt.Println("running for standard library")
	cloneWithFunc(slices.Clone[[]*int, *int])
	fmt.Println()
	fmt.Println("running for local func")
	cloneWithFunc(clone[[]*int, *int])
}

func cloneWithFunc(f func([]*int) []*int) {
	x := []*int{newInt(), newInt(), newInt()}
	x = f(x[:0])
	forceGC()
	fmt.Println("did the finalizer for this slice items run before this printed line?", x)
	forceGC()
}

func clone[S ~[]E, E any](s S) S {
	return append(S(nil), s...)
}

func forceGC() {
	runtime.GC()
	runtime.GC()
}

var count int

func newInt() *int {
	count++
	id := count
	x := new(int)
	runtime.SetFinalizer(x, func(*int) {
		fmt.Println("finalizer run for item:", id)
	})
	return x
}

What did you see happen?

running for standard library
did the finalizer for this slice items run before this printed line? []
finalizer run for item: 3
finalizer run for item: 2
finalizer run for item: 1

running for local func
finalizer run for item: 5
finalizer run for item: 4
finalizer run for item: 6
did the finalizer for this slice items run before this printed line? []

What did you expect to see?

running for standard library
finalizer run for item: 3
finalizer run for item: 2
finalizer run for item: 1
did the finalizer for this slice items run before this printed line? []

running for local func
finalizer run for item: 5
finalizer run for item: 4
finalizer run for item: 6
did the finalizer for this slice items run before this printed line? []

[diegommm]

Open to contribute with a CL in case this can be confirmed.

[gabyhelp]

Related Issues and Documentation

• cmd/compile: suboptimal cloning/optimization in slices.Clone #53643
• cmd/compile: a slice captured by a closure might leak #48254 (closed)
• cmd/compile: stringtoslicebytetmp optimization on unescaped slice #38501 (closed)
• x/exp/slices: Clone returns nil for an empty non-nil slice #49660 (closed)
• cmd/compile: unsafe.{SliceData,StringData} possible memory corruption in escape analysis #57823 (closed)
• Garbage Collection with References Leak #51138 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[Jorropo]

See this modified version: https://go.dev/play/p/uSY4HsNVTjQ

running for standard library
did the finalizer for this slice items run before this printed line? {0xc000010018 0 0}
finalizer run for item: 3
finalizer run for item: 2
finalizer run for item: 1

running for local func
finalizer run for item: 4
finalizer run for item: 6
finalizer run for item: 5
did the finalizer for this slice items run before this printed line? {<nil> 0 0}

As well as:

go/src/slices/slices.go

Lines 348 to 349 in 90c6558

	// The s[:0:0] preserves nil in case it matters.
	return append(s[:0:0], s...)

the behaviors are different.

I'll submit a fix thx for the report.

[diegommm]

Jorropo changed the title slices: make Clone allow gc of non-copied items slices: Clone keeps source array alive when source slice is zero length

@Jorropo this also happens when the source slice is non-zero length. In cloneWithFunc, try changing x = f(x[:0]) for x = f(x[:1]). Starting from your version, I get:

running for standard library
finalizer run for item: 3
finalizer run for item: 2
did the finalizer for this slice items run before this printed line? {0xc00004a028 1 1}
finalizer run for item: 1

running for local func
finalizer run for item: 6
finalizer run for item: 5
did the finalizer for this slice items run before this printed line? {0xc00004a000 1 1}
finalizer run for item: 4

[Jorropo]

@diegommm the arrays don't overlap when using x[:1] https://go.dev/play/p/GpymLHZOQFL
Finalizers are janky (such as not being synchronous and reviving references)
I don't think the output above can be interpreted because both the std bugged version and your fixed version give the same output.

[gopherbot]

Change https://go.dev/cl/598875 mentions this issue: slices: prevent Clone keeping alive the array when cloning empty slices

[ianlancetaylor]

I'm not convinced that this is worth fixing. Finalizers don't make any promises. Calling slices.Clone on an empty slice is an unusual action. This seems more like something to cover in a blog post somewhere.

[tdakkota]

@ianlancetaylor

I would expect that slices.Clone always returns slice pointing to a new location.
People might use it in a same way as strings.Clone: to make a copy that does not point
to a larger slice.

[diegommm]

@ianlancetaylor Could you clarify what do you mean by "worth fixing"? Do you see any downside of applying this fix?

[ianlancetaylor]

On the CL @randall77 gave a good rationale for the change, and I'm OK with it.

[zigo101]

There is not a perfect clone implementation in Go.

I agree that simplicity is not important for the implementation of the slices.Clone function.

[zigo101]

On the other hand, I think Go runtime should specially handle zero-capacity slicing results, along with taking addresses of zero-size values, to make the data pointers point to a global unique address within each program run. Doing this will remove many surprises in Go programming.

[edit] Doing this will benefit the cases shown in this issue and in https://go.dev/play/p/ENRf3aUWhV2 and https://go.dev/play/p/kBhZIEk77Dz

[diegommm]

@zigo101 would it make sense instead to just use a nil pointer instead of using a global unique address for all zero-size values?

[zigo101]

In Go, SliceType{} and SliceType(nil) are different. Similarly, a pointer to a zero-size value is not a nil pointer.

[muktihari]

I was asking this kind of question in golang-nuts since I didn't know this issue is already exist, thanks @randall77 for pointing me to this issue. There is already CL (https://go-review.googlesource.com/c/go/+/598875) for this, thanks for that.

I just want to share a real world use case that I come across when I try implementing Garmin's FIT Protocol encoding in Go, FIT SDK for Go, in case this may add another rationale why the fix is matter. Here is the context:

In a nutshell, FIT file is a binary file format that consist of multiple messages, and each message has fields:
layout: message, len(fields), fields...

record,4,timestamp,distance,latitude,longitude
record,2,timestamp,distance
...

Event though there is len(fields) "n", I can't simply use make([]Field, n), since each field may has components that expand into fields, and the field created from component may as well require expansion. Calculating the total of the fields is more expensive than just copying a slice, this is where I need to use array pool. The code in a nutshell:

mesg.Fields := d.fieldsArray[:0] // big enough to hold all fields
d.decodeFields(&mesg)
for _, field := range mesg.Fields {
    // d.expandComponent(mesg, field):
    value := field.Value
    for _, component := range field.Components {
        // Expand until value is depleted.
        if value == 0 {
            break
        }

        compField := factory.CreateField(mesg.Num, component.Num)
        mask := (1 << component.Bits) - 1
        compField.Value = value & mask
        value = value >> component.Bits

        mesg.Fields = append(mesg.Fields, compField)

        if len(compField.Components) > 0 {
            d.expandComponent(mesg, compField) // Recursive
        }
    }
}
mesg.Fields = append(mesg.Fields[:0:0], mesg.Fields...) // slices.Clone

Now, this is the related part of this issue, if by any chance len(fields) of a message is zero, the resulting messages may contains a message that still pointing to d.fieldsArray, this may cause the Decoder{} (or maybe array in sync.Pool if we ever use it) can't be freed until that message is no longer used. At scale, this may affect performance even though the chance of encountering that kind of file is uncommon, but it's not zero.

And also, in my opinion, having slices.Clone is just simply append(s[:0:0], s...) without optimization is only saving us a couple of character to type, but with respect for that now I know that I can do s[:0:0] variant in case I need it, I didn't think about it before. Thank you.