Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: CopyFS should guard against copying into the source directory #70465

Open
WinXaito opened this issue Nov 20, 2024 · 9 comments
Open

os: CopyFS should guard against copying into the source directory #70465

WinXaito opened this issue Nov 20, 2024 · 9 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@WinXaito
Copy link

WinXaito commented Nov 20, 2024

Go version

1.23.3

Output of go env in your module/workspace:

Output of `go env`
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: #62484

@gabyhelp
Copy link

Related Issues

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

@seankhliao
Copy link
Member

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.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2024
@WinXaito
Copy link
Author

WinXaito commented Nov 20, 2024

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
Copy link
Contributor

Do you want to send a patch for the documentation?

@WinXaito
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

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

@seankhliao seankhliao reopened this Nov 20, 2024
@seankhliao seankhliao changed the title os: os.CopyFS - Infinite loop when referencing to itself os: CopyFS should guard against copying into the source directory Nov 20, 2024
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 20, 2024
@dmitshur dmitshur added this to the Backlog milestone Nov 20, 2024
@ianlancetaylor
Copy link
Contributor

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.

@jfrech
Copy link

jfrech commented Nov 23, 2024

I would dislike [os.CopyFS] trying to be clever and sometimes not following its one-sentence documentation "CopyFS copies the file system fsys into the directory dir, creating dir if necessary.".

To the list of valid uses which may imply non-halting behaviour I will add copying from an lazily-infinite [io/fs.FS].

So I would say even the implementation approach of first walking fsys in its entirety before starting to copy (which would 'detect' sub-filesystems) isn't expected behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants