Previously: #3358

cc @zx2c4 @alexbrainman @mattn

@bsamek I can reproduce it here, thank you very much.

The problem is that

os.RemoveAll("foo")

calls

os.Remove(`foo\bar\aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb`)

which calls

syscall.RemoveDirectory(`foo\bar\aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb`)

which fails, because syscalls don't allow for relative path to be longer than 256 chars. But syscall.RemoveDirectory should succeeds, because directory exists and can be deleted.

Path is converted with os.fixLongPath in os.Remove, but os.fixLongPath does not handle relative path, and returns them as is.

Alex

Could os.RemoveAll() construct an absolute path and pass that to os.Remove() ?

Change https://golang.org/cl/214437 mentions this issue: os: handle long path in RemoveAll for windows

I don't think that fix is correct...

Change https://golang.org/cl/214598 mentions this issue: os: actually remove long path in TestRemoveAll

Change https://golang.org/cl/214601 mentions this issue: Revert "os: handle long path in RemoveAll for windows"

Patch was reverted, so reopening issue.

Converting a relative path to absolute path on Windows is also discussed here.
AFAIK, syscall package is frozen for Windows and only existing calls are available.

In this peculiar case, the end of the code can become until RemoveAll is improved:

// remove the root of the long path
	fp, err := filepath.Abs("foo")
	if err != nil {
		log.Fatalln("abs:", err)
	}
	err = os.RemoveAll(fp)
	if err != nil {
		log.Fatal(err)
	}

Regarding RemoveAll, adding a Windows specific file is considered here but issue was closed after updating Unix-like builds.

We can create a Windows-specific os.RemoveAll. Maybe it can Chdir before removing directory entries, instead of using path+\+file .

The syscall package can be updated to fix bugs in the stdlib.

Can you elaborate on your code segment above, where would that live?

It does not seem that the syscall package has an issue as related syscall methods fail appropriately when using the example. RemoveAll hangs probably because of some recursion issue. Handling some of the possible errors requires probably to be tailored to Windows behavior.

package main

import (
	"log"
	"os"
	"path/filepath"
	"strings"
	"syscall"
)

func main() {
	// make a long path
	wd, err := os.Getwd()
	if err != nil {
		log.Fatal(err)
	}
	// relative creation fails on a path longer than MAX_PATH
	fp := filepath.Join(wd, "foo", "bar", strings.Repeat("a", 150), strings.Repeat("b", 150))
	err = os.MkdirAll(fp, 0755)
	if err != nil {
		log.Fatal(err)
	}

	// remove the root of the long path using syscall
	p, e := syscall.UTF16PtrFromString("foo")
	if e != nil {
		log.Fatalln(e)
	}
	err = syscall.RemoveDirectory(p)
	if err == syscall.ERROR_DIR_NOT_EMPTY {
		log.Println("relative RemoveDirectory failed as expected as directory is not empty")
	} else if err != nil {
		log.Println(err)
	}

	// Moving to full path
	fps, ef := syscall.FullPath("foo")
	if ef != nil {
		log.Fatalln(ef)
	}
	if fps != fp[0:len(fps)] {
		log.Fatalf("fullpath: got %s, want %s", fps, fp[0:len(fps)])
	}

	p, e = syscall.UTF16PtrFromString(fps)
	if e != nil {
		log.Fatalln(e)
	}
	err = syscall.RemoveDirectory(p)
	if err == syscall.ERROR_DIR_NOT_EMPTY {
		log.Println("absolute RemoveDirectory failed as expected as directory is not empty")
	} else if err != nil {
		log.Fatal(err)
	}
}

What happens for syscall.RemoveDirectory("foo\bar\a...\b...") ? I think that's where the hang must occur, since os.RemoveAll() tries that path for os.RemoveAll("foo")

EDIT: It could also hang in the syscall within os.Lstat(). And within os.Chdir() if we tried that instead of "path+\+file". So we need a solution re long relative paths for the whole os package. We should at least return an error if a long path cannot be made absolute doesn't start with \\?\. Maybe some help can be found here:
https://googleprojectzero.blogspot.com/2016/02/the-definitive-guide-on-win32-to-nt.html

More here: microsoft/winfile#50 (see comments by @malxau)

EDIT: and https://stackoverflow.com/questions/38036943/getfullpathnamew-and-long-windows-file-paths

Indeed, full path names are required. The code below shows which error messages current RemoveAll receives.

output is amended for readability

$GOPATH\{some work path}>go run openlongpath.go
2020/02/08 07:47:02 Lstat failed with IsNotExist error
2020/02/08 07:47:03 CreateFile foo\bar\a...a\b....b: The system cannot find the path specified.
2020/02/08 07:47:03 Open failed with IsNotExist error
2020/02/08 07:47:03 open foo\bar\a...a\b....b: The system cannot find the path specified.
exit status 1

$GOPATH\{some work path}>dir /B /S foo
$GOPATH\{some work path}\foo\bar
$GOPATH\{some work path}\foo\bar\a...a
$GOPATH\{some work path}\foo\bar\a...a\b...b

package main

import (
	"log"
	"os"
	"path/filepath"
	"strings"
)

func main() {
	// make a long path
	wd, err := os.Getwd()
	if err != nil {
		log.Fatal(err)
	}
	// relative part of the full path
	fp := filepath.Join("foo", "bar", strings.Repeat("a", 150), strings.Repeat("b", 150))
	// creation requires to use an absolute path
	err = os.MkdirAll(filepath.Join(wd, fp), 0200)
	if err != nil {
		log.Fatal(err)
	}


	// Lstat using relative path
	var fh os.FileInfo
	fh, err = os.Lstat(fp)
	if os.IsNotExist(err) {
		log.Println("Lstat failed with IsNotExist error")
		log.Println(err)
	} else if err != nil {
		log.Fatal(err)
	}
	if err == nil && fh.IsDir() {
		log.Printf("%s... is a directory\n", fh.Name())
	}

	// Open using relative path
	var f *os.File
	f, err = os.Open(fp)
	if os.IsNotExist(err) {
		log.Println("Open failed with IsNotExist error")
		log.Fatal(err)
	} else if err != nil {
		log.Fatal(err)
	}
	fh, err = f.Stat()
	if err == nil && fh.IsDir() {
		log.Printf("%s... is a directory\n", fh.Name()[0:4])
	}
	if err != nil {
		log.Fatal(err)
	}

}

AFAIK, using full path is not enough to fix hanging. Lstat is required to know if the path is a directory. A file handle is also required to list directories using Readdirnames(). All add concurrency on the file structure.

From Microsoft documentation of RemoveDirectory, recursion requires to use shfileoperation which should probably be added to internal/syscall/windows package..

Have you tested os.Remove with a long absolute path, both empty and non-empty? I thought that was verified to work.

We don't need os.Remove to do recursion, we expect it to return an error if it's a non-empty directory.

I suggested two possible solutions here: #21782 (comment)

RemoveDirectory is using windows function RemoveDirectoryW. It says:

In the ANSI version of this function, the name is limited to MAX_PATH characters. To extend this limit to 32,767 wide characters, call the Unicode version of the function and prepend "\\?\" to the path. For more information, see Naming a File.

Prepending "\\?\" shouldn't be enough to fix this?

Yes, I suggested that in the issue referenced above, which we should fix at the same time. Feel free to submit a patch. (Note that you have to sign a CLA first.)

@gopherbot add "help wanted"

Note that prepending \\?\ changes several aspects about how the path is evaluated. This may or may not be a problem, but it requires conscious thought about each one:

• Relative components are not evaluated
• Trailing periods and spaces are not removed
• Special DOS device names (CON, AUX, PRN etc) are treated as files rather than devices
• The format of SMB paths is different (\\?\UNC\server\share vs \\server\share)

In the past when I've done this it becomes important to define some form of consistency. It'd be bad if "foo." evaluates to different files depending on the API being invoked.

Good point. Maybe the fix is to prepend \\?\ or \\?\UNC\ (if missing) to the name computed for each directory entry and passed to RemoveAll(), instead of to the name passed to Remove() which may be the original argument from the caller of os.RemoveAll(). Then it's up to the caller to prepend \\?\ in the original argument if nec.

I think the same logic applies to filepath.Walk().

cc @bcmills

There is a CL for this change, but it didn’t make it in early. Punting to Go1.17.

Change https://golang.org/cl/291291 mentions this issue: os: support long paths without fixup on Windows 10 >= 1607

CL 291291 was merged. What else is there to do for this issue? Thanks.

We could probably go in circles on this for a long time trying to handle what is now "old Windows", but given that it's no longer a problem into the future with Windows 10 >= 1607, maybe we can close this and not worry about the old stuff so much? That's a little less conservative than the Go project usually likes to be, but maybe it's not an all together bad approach for this?

I suggest leaving this open as long as earlier Windows versions are supported.

I just checked the program above #36375 (comment) now runs without any failures.

So this issue is fixed. Probably by CL 291291.

This is still broken for older versions of Windows.

But, I agree with Jason. I don't see anyone working to fix this issue on older Windows.

So I would close the issue. But I would let Ian to decide.

Alex

The fix is quite simple, and described here: #36375 (comment)

cc @rasky

CC @bufflig

Based on the conversation, I understand this is fixed for newer Windows, but not older.

It seems this might not make it to 1.17, so I'll keep the issue open but move it to Backlog for now.

In a comment above, @networkimprov suggests there's a simple fix that can be evaluated. @bufflig, if you have a chance to look at this before beta 1, please feel free to move it back to Go 1.17 milestone.

Since I tagged this issue, I'll explain why: although deletion due to filename-length is probably fixed in newer Windows due to long-filename support, other reasons for os.Remove returning an IsNotExist-matching error while the target file still exists will still produce an unterminating loop in os.RemoveAll.

One such case (the one that brought me here) is if the filename is not value UTF-16: the filename collected by (f *File) Readdirnames will not correctly round-trip back to match that file (utf16.Decode replaces unpaired surrogate code-points with the replacement character), and hence os.Remove will return an IsNotExist-matching error because it was given the name with replacement characters, which doesn't exist. Hence, a file-not-found error is returned, the loop thinks it deleted something, closes and reopens the directory entry list, and tries to delete the same file again to the same effect, as seen in this bug report.

One example of filenames with invalid surrogate pairs is the files that msys and cygwin create when their POSIX API is used to delete, e.g., the currently-running executable, but the underlying Win32 API cannot perform that deletion.

I'm not sure if this can be addressed in Go, short of implementing a Windows-specific os.RemoveAll that doesn't pass the filenames through a string, but maintains the []uint16 the whole way. That's basically what we're looking at doing in an external library, see microsoft/go-winio#261.

@TBBle thanks for reporting the problem with filenames containing unpaired surrogates. I think it deserves its own issue, could you create one with an small reproduces?

I don't have the immediate time to produce a minimal repro right now, but I'll point it to the unit test which demonstrates this in the hcsshim PR working around it, pending a standalone repro.

Change https://go.dev/cl/574695 mentions this issue: os: support relative paths in fixLongPath