os: Root can no longer be safely copied in Go 1.25

[fastcat]

Go version

1.25rc2

Output of go env in your module/workspace:

`go env` output, not particularly relevant

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/cheetah/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/cheetah/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1694165664=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/cheetah/src/gdev/go.mod'
GOMODCACHE='/home/cheetah/.go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/cheetah/.go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/cheetah/.go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25rc2.linux-amd64'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/cheetah/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/cheetah/.go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25rc2.linux-amd64/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.25rc2'
GOWORK='/home/cheetah/src/gdev/go.work'
PKG_CONFIG='pkg-config'

What did you do?

I (mistakenly) tried to do a struct embedding of an os.Root in my application, and then got confused when I got file already closed errors.

Since os.OpenRoot attaches a Cleanup to the *os.Root pointer returned that calls *os.Root.Close() once freed,, it's important that no struct copies be made, as those copies won't work properly.

What did you see happen?

file already closed errors when my os.Root instance was still live in active goroutine stacks

What did you expect to see?

Some combination of:

1. A documentation note that the struct is not safe to copy
2. A vet error telling me that same thing

[gabyhelp]

Related Issues

• os: `Root.OpenRoot` sets incorrect name, losing prefix of original root #73868

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

[TapirLiu]

Does the problem exist with toolchain 1.24?

Before 1.25, the cleaup setup code is

runtime.SetFinalizer(r.root, (*root).Close)

In 1.25rc2, it is

 r.root.cleanup = runtime.AddCleanup(r, func(f *root) { f.Close() }, r.root)

Maybe, it should be

 r.root.cleanup = runtime.AddCleanup(r.root, func(f *root) { f.Close() }, r.root)

?

[fastcat]

Aah, I hadn't checked for changes since 1.24, as my use case needs some of the new methods on os.Root in 1.25. That said, the code indeed looks like the problem was introduced in 1.25.

This change came from https://go.dev/cl/638555 / #70907 / fdaac84 ... FYI @cagedmantis

Your suggested fix won't work however, due to the rules on AddCleanup:

If ptr is reachable from cleanup or arg, ptr will never be collected and the cleanup will never run. As a protection against simple cases of this, AddCleanup panics if arg is equal to ptr.

[TapirLiu]

Ah, TIL.

One way is to make an extra indirect, like r.root.root. But it is really hilarious. :D

[fastcat]

Yes, that seems like it would be a bit out there for a solution :)

I don't know how many others might have code that does struct copying that would be broken by adding a "noCopy" member. Some folks have large corpuses of code they have been able to search for such things in the past, though I don't know if this would be an easy thing to search for.

I don't know how noCopy on things like this falls into the compatibility promise, but I feel like my struct copying here was likely to be a very rare thing to do.

[cagedmantis]

@neild @ianlancetaylor @bradfitz @griesemer @cagedmantis

[neild]

Adding noCopy to Root seems reasonable to me. You're definitely not suppose to copy a Root, any more than you should copy a File.

(File suffers from the same cleanup problem, FWIW, and is not marked noCopy.)

[apparentlymart]

From what's been discussed it seems like copying a Root is already subtly broken anyway, unless you're lucky enough that both of your copies end up getting collected at roughly the same time, and so marking as "no copy" would help people find existing bugs and fix them.

I'm also understanding this as being a new vet check rather than a new compile-time error, so it's not really a "breaking change" in the sense that the program won't compile anymore. It might be annoying for those who consider vet checks a blocker for merging changes, but existing code that previously worked will still compile.

[mknyszek]

I think we can fix this by changing the cleanup func. We also don't need another level of indirection.

At the point the cleanup func is called, (1) there can be no more mutex holders, (2) there's no point in setting root.closed, root is gone, and (3) same for r.refs. root is just gone at that point. Most of Close is useless then, except for the syscall.Close.

This is part of the point of using AddCleanup over SetFinalizer: just pass what you actually need. That's enough to break the cycle. And in this case I think all we actually need is the file descriptor.

In other words, what I'm proposing is changing

 r.root.cleanup = runtime.AddCleanup(r, func(f *root) { f.Close() }, r.root)

to

 r.root.cleanup = runtime.AddCleanup(r.root, func(fd sysfdType) { syscall.Close(fd) }, r.root.fd)

(With appropriate comments explaining why this is safe.)

[prattmic]

(File suffers from the same cleanup problem, FWIW, and is not marked noCopy.)

This sounds like a regression in https://go.dev/cl/638555. Previously we had a finalizer on the internal *file, but for cleanup we moved it to File, which I believe changes the type from safe to copy to unsafe.