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: RemoveAll hangs on a path longer than 260 characters on Windows #36375

Open
bsamek opened this issue Jan 3, 2020 · 31 comments
Open

os: RemoveAll hangs on a path longer than 260 characters on Windows #36375

bsamek opened this issue Jan 3, 2020 · 31 comments

Comments

@bsamek
Copy link

@bsamek bsamek commented Jan 3, 2020

On Go 1.13.5 on Windows 2008 R2, running os.RemoveAll on a directory that contains a path longer than 260 characters hangs.

In the following repro, the directory is created, and then the program hangs on os.RemoveAll.

package main

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

func main() {
	// make a long path
	a := ""
	b := ""
	for i := 0; i < 150; i++ {
		a += "a"
		b += "b"
	}
	wd, err := os.Getwd()
	if err != nil {
		log.Fatal(err)
	}
	err = os.MkdirAll(filepath.Join(wd, "foo", "bar", a, b), 0755)
	if err != nil {
		log.Fatal(err)
	}

	// remove the root of the long path
	err = os.RemoveAll("foo")
	if err != nil {
		log.Fatal(err)
	}
}
@ALTree
Copy link
Member

@ALTree ALTree commented Jan 3, 2020

Previously: #3358

@ALTree ALTree changed the title os.RemoveAll hangs on a path longer than 260 characters on Windows os: RemoveAll hangs on a path longer than 260 characters on Windows Jan 3, 2020
@ALTree ALTree added this to the Go1.15 milestone Jan 3, 2020
@networkimprov
Copy link

@networkimprov networkimprov commented Jan 4, 2020

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jan 4, 2020

@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

@networkimprov
Copy link

@networkimprov networkimprov commented Jan 4, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 12, 2020

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

@gopherbot gopherbot closed this in 5c44cc4 Jan 13, 2020
@networkimprov
Copy link

@networkimprov networkimprov commented Jan 13, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 13, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 13, 2020

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

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 13, 2020

Patch was reverted, so reopening issue.

gopherbot pushed a commit that referenced this issue Jan 13, 2020
This reverts CL 214437.

Does not fix the issue, and the test was wrong so it did not detect that it did not fix the issue.

Updates #36375

Change-Id: I6a4112035a1e90f4fdafed6fdf4ec9dfc718b571
Reviewed-on: https://go-review.googlesource.com/c/go/+/214601
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@iWdGo
Copy link
Contributor

@iWdGo iWdGo commented Jan 18, 2020

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.

@networkimprov
Copy link

@networkimprov networkimprov commented Jan 18, 2020

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?

@iWdGo
Copy link
Contributor

@iWdGo iWdGo commented Jan 26, 2020

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)
	}
}
@networkimprov
Copy link

@networkimprov networkimprov commented Jan 27, 2020

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

@iWdGo
Copy link
Contributor

@iWdGo iWdGo commented Feb 8, 2020

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)
	}

}
@iWdGo
Copy link
Contributor

@iWdGo iWdGo commented Feb 8, 2020

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

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 8, 2020

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)

@ackFacu
Copy link

@ackFacu ackFacu commented Apr 18, 2020

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?

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 18, 2020

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"

@malxau
Copy link

@malxau malxau commented Apr 18, 2020

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.

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 18, 2020

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

@ackFacu
Copy link

@ackFacu ackFacu commented Apr 18, 2020

Let me set the environment and read some more about the process for submitting code, and probably will make this my first collaboration to go code

@VirtualSkyRepo
Copy link

@VirtualSkyRepo VirtualSkyRepo commented Apr 19, 2020

Appending \\?\ is not a solution. Golang already does that for absolute paths, but explicity avoid doing this into relative path. I've readed a little more about this, and it looks like appending that only works for absolute paths. Relative paths always have to be len MAX_LENGHT

Because you cannot use the "\?" prefix with a relative path, relative paths are always limited to a total of MAX_PATH characters.
https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

The only solution would be to convert relative paths into absolute paths, if possible. I will investigate about this.

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 20, 2020

As mentioned in the comment re filepath.Walk() that I linked above, we can prepend \\?\ to the result of GetFullPathNameW() to create a valid long path; see:
https://stackoverflow.com/questions/38036943/getfullpathnamew-and-long-windows-file-paths

That needs to be done for directory entries, as described in my previous post.

@VirtualSkyRepo
Copy link

@VirtualSkyRepo VirtualSkyRepo commented Apr 20, 2020

Yes, sure. I've introduced a syscall.FullPath into fixLongPath, and now it works for most cases presented, but it is still failing in Walk, because the path that's failing is 227 characters long (<248), so it doesn't get processed by this. (This was commented in one of the first issues in filepath.Walk ticket, mentioned before). I have to investigate why this is failing, and fix this.

Cheers ;)

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 20, 2020

@quentinmit describes here why he didn't do that already: #21782 (comment)

I don't think we can touch fixLongPath(), because everything calls it, and we could easily break some case for which there isn't a test. We only need to fix filepath.Walk() and os.RemoveAll(), and we already know how to do so.

@VirtualSkyRepo
Copy link

@VirtualSkyRepo VirtualSkyRepo commented Apr 20, 2020

mmm.. But Is backwards logic I think.
The issue is in fixLongPath, It's not being able to convert relative paths.
You say to fix Walk and RemoveAll. But it's also failing for lstat and open. This is going to fail everywere fixLongPath is called.

fixLongPath is called in places were a fix for MAX_PATH is needed, but it's not working for relative paths.
As far as we know for now, this is failing in:
open, lstat, remove, removeAll and walk.

But I think that this list is way more extensive.
I think it's wrong to try to fix all them separately if the real issue is fixLongPath not working.

If we are not going to fix fixLongPath, we should be clear about this not working for relative paths, and functions like lstat, open or so, should be clear about not working for relative paths longer than 248 chars on windows.

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 20, 2020

See #20829 re long (i.e. invalid) relative path arguments. Users can adjust arguments they pass.

But a valid relative path can become invalid by concatenation during a tree walk. So we just need to fix what os.RemoveAll() passes to RemoveAll() (and likewise for filepath.Walk()) by defining a parent from GetFullPathNameW() with prepended \\?\ or \\?\UNC\ -- unless that's already present.

@VirtualSkyRepo
Copy link

@VirtualSkyRepo VirtualSkyRepo commented Apr 20, 2020

But ticket #20829 was fixed adding fixLongPath(name):
https://go-review.googlesource.com/c/go/+/47010/

Also, if we are not going to support for users to give us a relative path longer than MAX_PATH, then we should be returning a special error for that, instead of "not existent file/folder", that's not necesarily true

@networkimprov
Copy link

@networkimprov networkimprov commented Apr 20, 2020

Read the whole discussion therein.

Changing fixLongPath() to return an error for long relative paths is worth considering, but out of scope for this issue.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 23, 2020

We are already weeks into the code freeze, moving to Go1.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.