os: RemoveAll fails for long file paths #27029

ostenbom · 2018-08-16T13:53:40Z

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.

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 · 2018-08-21T01:33:18Z

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 · 2018-08-30T15:07:39Z

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

bradfitz · 2018-08-31T05:16:57Z

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

ostenbom · 2018-09-17T15:18:21Z

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:

Extending the syscall.Unlinkat signature to accept the flags param
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 · 2018-09-17T18:41:07Z

@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 · 2018-09-18T16:14:22Z

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:

Is this the right approach structurally?
What platforms to aim on fixing this for? All unix?
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 · 2018-09-18T17:14:57Z

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 · 2018-09-19T16:01:09Z

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 · 2018-09-19T16:36:25Z

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 · 2018-09-19T16:47:32Z

@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 · 2018-09-19T17:01:40Z

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.

On unix systems, long enough path names will fail on doing syscall like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux and darwin. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <julianedialkova@hotmail.com> Change-Id: I9d8a634bb7a479e5ef80fc346aeff478bde1f607

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux and darwin. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <julianedialkova@hotmail.com> Change-Id: Ibfa8fd2b60f49c34ad5468a13f9d459d32fe8af5

ianlancetaylor · 2018-09-25T03:26:10Z

@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.

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux and darwin. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com> Change-Id: I21730a61c70a38b2fcef8dbab6cae4eca3f95f05

gopherbot · 2018-09-26T08:59:03Z

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

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux and darwin. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com> Change-Id: I6a965569beb113c10a52967d5ea0294fcd8dd9d6

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux and darwin. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com> Change-Id: I80ed34170fd826c008d596f36a1534ea40a17ea6

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin, freebsd and openbsd. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com> Change-Id: I77dfe62a28d86e7cbcfb5aa0b19a7651572c416b

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin, freebsd and openbsd. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com>

gopherbot · 2018-10-30T15:28:07Z

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

bradfitz · 2018-10-30T16:56:19Z

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

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin, freebsd and openbsd. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com>

gopherbot · 2018-10-30T23:05:18Z

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

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin, freebsd and openbsd. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com>

On unix systems, long enough path names will fail when performing syscalls like `Lstat`. The current RemoveAll uses several of these syscalls, and so will fail for long paths. This can be risky, as it can let users "hide" files from the system or otherwise make long enough paths for programs to fail. By using `Unlinkat` and `Openat` syscalls instead, RemoveAll is safer on unix systems. Initially implemented for linux, darwin and openbsd. Not yet implemented on freebsd due to fstatat 64-bit inode compatibility issues. Fixes golang#27029 Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io> Co-authored-by: Julia Nedialkova <yulia.nedyalkova@sap.com>

Follow CL 146020 and enable RemoveAll based on Unlinkat and Openat on solaris. Updates #27029 Change-Id: I0b0e92f4422fa960a13dcd3e9adb57cd23f09ed4 Reviewed-on: https://go-review.googlesource.com/c/145839 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot · 2018-11-01T08:26:58Z

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

Follow CL 146020 and enable RemoveAll based on Unlinkat and Openat on freebsd. Since the layout of syscall.Stat_t changes in FreeBSD 12, Fstatat needs a compatibility wrapper akin to Fstatat in x/sys/unix. See CL 138595 and CL 136816 for details. Updates #27029 Change-Id: I8851a5b7fa658eaa6e69a1693150b16d9a68f36a Reviewed-on: https://go-review.googlesource.com/c/146597 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Yuval Pavel Zholkover <paulzhol@gmail.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2018-11-02T08:27:06Z

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

Follow CL 146020 and enable RemoveAll based on Unlinkat and Openat on aix. Updates #27029 Change-Id: I78b34ed671166ee6fa651d5f2025b88548ee6c68 Reviewed-on: https://go-review.googlesource.com/c/146937 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Clément Chigot <clement.chigot@atos.net> Reviewed-by: Ian Lance Taylor <iant@golang.org>

ALTree changed the title ~~os.RemoveAll fails for long file paths.~~ os: RemoveAll fails for long file paths Aug 16, 2018

ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 16, 2018

ALTree added this to the Go1.12 milestone Aug 16, 2018

ttaylorr mentioned this issue Sep 25, 2018

Git-LFS can't work with complex path on Windows 10 git-lfs/git-lfs#2782

Closed

ostenbom mentioned this issue Sep 26, 2018

os: add support for long path names on unix RemoveAll #27871

Closed

gopherbot closed this as completed in 85143d3 Oct 30, 2018

bradfitz reopened this Oct 30, 2018

ostenbom mentioned this issue Oct 30, 2018

os: add support for long path names on unix RemoveAll #28494

Closed

gopherbot closed this as completed in e8ffb8a Oct 31, 2018

cespare mentioned this issue Mar 4, 2019

os: RemoveAll doesn't remove nested un-readable directories #30555

Closed

iwdgo mentioned this issue Jan 18, 2020

os: RemoveAll hangs on a path longer than 260 characters on Windows versions before Windows 10 1607 #36375

Closed

golang locked and limited conversation to collaborators Mar 4, 2020

gopherbot added the FrozenDueToAge label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: RemoveAll fails for long file paths #27029

os: RemoveAll fails for long file paths #27029

ostenbom commented Aug 16, 2018 •

edited

ALTree commented Aug 16, 2018

bradfitz commented Aug 16, 2018

ianlancetaylor commented Aug 17, 2018

ostenbom commented Aug 20, 2018 •

edited

ianlancetaylor commented Aug 21, 2018

ostenbom commented Aug 30, 2018

bradfitz commented Aug 31, 2018

ostenbom commented Sep 17, 2018

ianlancetaylor commented Sep 17, 2018

ostenbom commented Sep 18, 2018

ianlancetaylor commented Sep 18, 2018

ostenbom commented Sep 19, 2018

mirtchovski commented Sep 19, 2018

ostenbom commented Sep 19, 2018

mirtchovski commented Sep 19, 2018

ianlancetaylor commented Sep 25, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Oct 30, 2018

bradfitz commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Nov 1, 2018

gopherbot commented Nov 2, 2018

os: RemoveAll fails for long file paths #27029

os: RemoveAll fails for long file paths #27029

Comments

ostenbom commented Aug 16, 2018 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

Suggested solutions

ALTree commented Aug 16, 2018

bradfitz commented Aug 16, 2018

ianlancetaylor commented Aug 17, 2018

ostenbom commented Aug 20, 2018 • edited

1. Patch os _unix files

2. Have a unix specific RemoveAll recursion

ianlancetaylor commented Aug 21, 2018

ostenbom commented Aug 30, 2018

bradfitz commented Aug 31, 2018

ostenbom commented Sep 17, 2018

ianlancetaylor commented Sep 17, 2018

ostenbom commented Sep 18, 2018

ianlancetaylor commented Sep 18, 2018

ostenbom commented Sep 19, 2018

mirtchovski commented Sep 19, 2018

ostenbom commented Sep 19, 2018

mirtchovski commented Sep 19, 2018

ianlancetaylor commented Sep 25, 2018

gopherbot commented Sep 26, 2018

gopherbot commented Oct 30, 2018

bradfitz commented Oct 30, 2018

gopherbot commented Oct 30, 2018

gopherbot commented Nov 1, 2018

gopherbot commented Nov 2, 2018

ostenbom commented Aug 16, 2018 •

edited

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

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

ostenbom commented Aug 20, 2018 •

edited

1. Patch os `_unix` files

2. Have a unix specific `RemoveAll` recursion