os: RemoveAll fails for long file paths

[ostenbom]

What version of Go are you using (go version)?

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Linux, amd64

What did you do?

Make a long file path (> 4096 chars). os.RemoveAll(path).

package main

import (
        "io/ioutil"
        "os"
        "path/filepath"
        "syscall"
        "fmt"
)

func main() {
        tmpDir, err := ioutil.TempDir("", "")
        if err != nil {
                panic(err)
        }
        err = createVeryLongFilePath(tmpDir)
        if err != nil {
                panic(err)
        }

        fmt.Println(tmpDir)
        err = os.RemoveAll(tmpDir)
        if err != nil {
                panic(err)
        }
}

func createVeryLongFilePath(startPath string) error {
        currentPath := startPath
        for i := 0; i < 41; i++ {
                name := ""
                for j := 0; j < 100; j++ {
                        name = name + "a"
                }
                if err := mkdirAt(currentPath, name); err != nil {
                        return err
                }
                currentPath = filepath.Join(currentPath, name)
        }

        return nil
}

func mkdirAt(atPath, path string) error {
        fd, err := syscall.Open(atPath, syscall.O_RDONLY, 0)
        if err != nil {
                return err
        }
        defer syscall.Close(fd)

        return syscall.Mkdirat(fd, path, 755)
}

What did you expect to see?

Success.

What did you see instead?

it fails with file name too long.

Suggested solutions

We have worked around this using unix.Openat and unix.Unlinkat in our code base, but this solution would only work on unix.

The problem really is with os.Remove, whose unix syscalls don't accept long file paths. An alternate solution would be to alter os.Remove to split long path names on unix systems.

We'd be happy to submit pull requests for either approach.
cc @Callisto13

[ALTree]

Reproducible on 1.11 too, leaving this for the 1.12 cycle since it's not a recent regression.

[bradfitz]

Nice bug, thanks! (When I saw the title I feared this was our Windows long paths bug creeping up again and I was afraid we hadn't fixed it correctly the first time. Glad to see it's a new bug and amused that it's on Linux.)

/cc @ianlancetaylor

[ianlancetaylor]

This is pretty hard to fix in a portable way, but I agree that we should probably start using unlinkat on systems that support it.

[ostenbom]

Some more thoughts on this:

File name too long is what many unix sycalls return when you give them paths over 4096 chars.

The following fails with the same error. Also if you fmt.Println(longPath) and try to mkdir your 4100 character long path you get the same error.

package main

import (
	"os"
	"path/filepath"
)

func main() {
	longPath := createVeryLongFilePath()

	_, err := os.Lstat(longPath)
	if err != nil {
		panic(err)
	}
}

func createVeryLongFilePath() string {
	currentPath := "/"
	for i := 0; i < 41; i++ {
		name := ""
		for j := 0; j < 100; j++ {
			name = name + "a"
		}
		currentPath = filepath.Join(currentPath, name)
	}

	return currentPath
}

File systems usually have a maximum filename length of 255 chars and linux has a max filepath limit of 4096. The same limit seems to apply to Darwin. I would argue that regardless of the filepath limit, RemoveAll shouldn't fail as could cause security problems. In our case for example, it allowed users to create containers with long filepaths that we then couldn't delete - which could have caused a DOS attack.

This leaves a few options:

1. Patch os _unix files

Patch stat_unix.go and file_unix.go. The Lstat, Open and Remove functions of the unix variant could be altered to deal with long file paths. For example:

func UnixRemoveWithTooLong(path string) error {
	if len(path) < 4096 {
		return Remove(path)
	}

	pathParts := strings.Split(path, "/")
	pathCentre := int(len(pathParts) / 2)

	firstPart := strings.Join(pathParts[:pathCentre], "/")
	secondPart := strings.Join(pathParts[pathCentre:], "/")


	// Or use unlinkat etc.
	Chdir(firstPart)
	UnixRemoveWithTooLong(filepath.Join(".", secondPart) // Note recursion
}

There might be some duplicated logic here, or inconsistencies between some unix sycalls supporting long filepaths and some not (unless more unix package functions are patched). This might also open up the possibility of creating long file paths - is that desired?

2. Have a unix specific RemoveAll recursion

For the RemoveAll recursion to work on unix, you'd have to change the unix function definition to func removeAll(parentFile *os.File, path string) error. The unix function could use unix.Openat and unix.Unlinkat to recurse using file descriptors. The error codes from Unlinkat can be used to follow the same error flow as in the current os.RemoveAll implementation. This is similar to our fix

The unix RemoveAll function would diverge from the os.RemoveAll in this case, which could make future improvements more complex.

[ianlancetaylor]

I don't think we need to support the general case of very long file names for os.Stat, os.Remove, and friends. I think that os.RemoveAll is a special case for which we should consider long file name support.

[ostenbom]

Okay I'd be happy to submit just a RemoveAll fix. @ALTree @bradfitz what do you think?

[bradfitz]

Sounds fine. We can always do more later if needed.

[ostenbom]

We've been looking at a fitting implementation here. For this to be solved, a Unlinkat(dirfd, path string, flags int) and an Openat(dirfd, path string, flags int, mode uint32) would be needed on unix platforms. Currently the Unlinkat that is exposed in the syscall package is lacking the flags argument, and both are only exposed on Linux. How do you feel about:

1. Extending the syscall.Unlinkat signature to accept the flags param
2. Exposing syscall.Unlinkat and syscall.Openat on other unixes?

The x/sys/unix package already does both these things, but from our understanding they are not to be used in the stdlib.

/cc @gcapizzi

[ianlancetaylor]

@ostenbom Thanks for looking into this. Our preference where the standard library requires system call support that is not presently in the syscall package is to add that support to the internal/syscall/unix package. That way we can let the standard library do what it needs without adding new API to the syscall package.

[ostenbom]

Ok @ianlancetaylor @bradfitz, we're working on a version according to your comments.
A draft here. We implemented Unlinkat and Openat in the internal package, and moved RemoveAll to its own file in os. It can then be possible to fill in platform-specific implementations of Unlinkat & Openat, and add supported platforms to os/removeall_unix.go's build comment.

There is a removeall.go with the normal implementation, and a removeall_unix.go, specifically for the unix fix.

The questions we have are:

1. Is this the right approach structurally?
2. What platforms to aim on fixing this for? All unix?
3. What to do with the test that tests RemoveAll for long path names? Skip on GOOS = unsupported

Other resources on understanding how the // +build comments and file_platform.go compilation works would help a lot!

Thanks

cc @gcapizzi

[ianlancetaylor]

Thanks. Seems like the right basic idea. removeall_unix.go is an OK name but perhaps the other file should be removeall_nonunix.go. Or perhaps there is a better implementation possible for Windows, I don't really know, in which case we would have removeall_unix.go, removeall_windows.go, removeall_plan9.go.

Since unlinkat is in POSIX.1-2001, it seems plausible that it should work for all Unix systems, and that should be our goal. It's OK if the initial CL doesn't support all Unix systems, though.

For the test, yes, skip it for systems that don't support it.

Build constraints are documented at https://golang.org/pkg/go/build .

[ostenbom]

Ok, great. We have a working implementation now that passes all tests, in the same branch. The only problem we have now is knowing the trap numbers for the Unlinkat and Openat and the AT_REMOVEDIR flag constant. Browsing through x/sys/unix it seems to vary a fair amount between platforms and architectures.

We started making link_sysnum_platform.go etc, but might need like 20 different files for these.. Any ideas of how to generate these or where else to find this tribal knowledge?

[mirtchovski]

I'm sorry for jumping in, but just wanted to mention that some time ago we had a similar problem where files could "hide" in directories way past PATH_MAX. This is exacerbated on macOS as PATH_MAX is just 1024 there. Just removing deep files wasn't an option: we wanted to collect information on them and see what they were.

We solved the problem generally by writing a walker that iteratively navigated deep directories by changing CWD. File operations on any files found that deep were done locally, no attempts to pass a very large filename through a syscall.

This worked on all operating systems supported by Go. Code is here: github.com/mirtchovski/walk

[ostenbom]

@mirtchovski Right doing the recursive chdir would rid us of long path names, but would probably have concurrency issues if, say, several goroutines were calling RemoveAll on the same directory.

[mirtchovski]

It definitely doesn't work well with concurrent walks. We usually do normal walking for most things and only trigger the deep path if we encounter nested directories beyond a certain depth. Usually in a separate process.

[ianlancetaylor]

@ostenbom The simplest approach is to write the code so that it falls back to normal behavior if the unlinkat system call number is not known, and then let various platform maintainers fill in the system call number at their leisure.

Also note that on many systems the number is already available as syscall.SYS_UNLINKAT. In fact that may cover all systems other than Solaris.

[gopherbot]

Change https://golang.org/cl/137442 mentions this issue: os: add support for long path names on unix RemoveAll

[gopherbot]

Change https://golang.org/cl/145839 mentions this issue: os: add support for long path names on solaris RemoveAll

[bradfitz]

This was reverted by https://go-review.googlesource.com/c/go/+/145897 (which didn't reference this issue).

[gopherbot]

Change https://golang.org/cl/146020 mentions this issue: os: Add support for long path names on unix RemoveAll

[gopherbot]

Change https://golang.org/cl/146597 mentions this issue: os: add support for long path names on freebsd RemoveAll

[gopherbot]

Change https://golang.org/cl/146937 mentions this issue: os: add support for long path names on aix RemoveAll