os: CopyFS should guard against copying into the source directory

WinXaito commented 1 day ago

Go version

1.23.3

Output of `go env` in your module/workspace:

Output of `go env`

```shell set GO111MODULE=on set GOARCH=amd64 set GOBIN= set GOCACHE=C:\Users\**REDACTED**\AppData\Local\go-build set GOENV=C:\Users\**REDACTED**\AppData\Roaming\go\env set GOEXE=.exe set GOEXPERIMENT= set GOFLAGS= set GOHOSTARCH=amd64 set GOHOSTOS=windows set GOINSECURE= set GOMODCACHE=C:\Users\**REDACTED**\go\pkg\mod set GONOPROXY=**REDACTED** set GONOSUMDB=**REDACTED** set GOOS=windows set GOPATH=C:\Users\**REDACTED**\go set GOPRIVATE=**REDACTED** set GOPROXY=https://proxy.golang.org,direct set GOROOT=C:/Users/**REDACTED**/go/go1.23.3 set GOSUMDB=sum.golang.org set GOTMPDIR= set GOTOOLCHAIN=auto set GOTOOLDIR=C:\Users\**REDACTED**\go\go1.23.3\pkg\tool\windows_amd64 set GOVCS= set GOVERSION=go1.23.3 set GODEBUG= set GOTELEMETRY=local set GOTELEMETRYDIR=C:\Users\**REDACTED**\AppData\Roaming\go\telemetry set GCCGO=gccgo set GOAMD64=v1 set AR=ar set CC=gcc set CXX=g++ set CGO_ENABLED=1 set GOMOD=**REDACTED** set GOWORK= set CGO_CFLAGS=-O2 -g set CGO_CPPFLAGS= set CGO_CXXFLAGS=-O2 -g set CGO_FFLAGS=-O2 -g set CGO_LDFLAGS=-O2 -g set PKG_CONFIG=pkg-config set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\**REDACTED**\AppData\Local\Temp\go-build2571252335=/tmp/go-build -gno-record-gcc-switches ```

What did you do?

Minimal reproductible example:

Create a folder structure like that:

- main.go
- test/
  - loop/
  - file1.txt

package main

import (
    "log"
    "os"
    "time"
)

func main() {
    go func() {
        if err := os.CopyFS("test/loop/", os.DirFS("test/")); err != nil {
            log.Fatalln(err)
        }
    }()

    time.Sleep(time.Millisecond * 200)
    os.Exit(0)
}

What did you see happen?

After running the minimal reproductible example, (I volontarily limitated it to 200ms in a goroutine, because it happen to be an infinite loop), approximately 200 files and folder are created, because the folder loop is copied in itself in an infinite loop.

What did you expect to see?

On windows, if we try to copy a directory in itself, we have a Warning dialog and the other files/folders are still copied.

I should expect that os.CopyFS do the copy but ignore if it is itself. Or return an error, but it could be annoying as we don't know if the rest of the files have been copied.

Reference issue of os.CopyFS: https://github.com/golang/go/issues/62484

gabyhelp commented 1 day ago

Related Issues

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

seankhliao commented 1 day ago

I think this is working as intended, CopyFS won't have the ability to peek into what the given abstract filesystem is, especially if the given filesystem is wrapped.

WinXaito commented 1 day ago

I understand that, so, definitly the developer has to do a check before.

But this behaviour is not documentated (at least as I saw).

And since it do an infinite loop and will stuck the program, it can also take down the whole server. And for me, this should not happen and it is dangerous.

ianlancetaylor commented 1 day ago

Do you want to send a patch for the documentation?

WinXaito commented 1 day ago

Do you want to send a patch for the documentation?

Yes, I can. Should I just open a merge request on Github ?

Otherwise, is there really no way implement something ?

I was thinking, maybe copy into a temporary directory and rename it to the good place. (I tried and seems to do the job). (It's kinda dirty, I agree).

I was also thinking, maybe by checking if fsys fs.FS is dirFS since it is in the same package and check in that case is path are relative to other. So with that it's maybe possible to have something clean ? Unfortunately it's not that easy for me since dirFS is private in os package.

ianlancetaylor commented 1 day ago

You can open a pull request on GitHub, or you can use Gerrit. See https://go.dev/doc/contribute. Thanks.

I don't think we want to copy to a temporary directory. That penalizes every caller for a rare mistake.

I think it would be OK for os.CopyFS to do a sanity check if the fs.FS is a os.dirFS. We could add something like

    if osDirFS, ok := fsys.(dirFS); ok {
        // check for nesting between osDirFS and dir
    }

seankhliao commented 1 day ago

seems it would be quite a fragile check if someone were to use say fs.Sub

ianlancetaylor commented 1 day ago

I agree that would be fragile. I haven't been able to think of any loop detector that would be reliable without preventing valid uses.

golang / go