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: handle relative paths in fixLongPath on Windows #41734

Open
rsc opened this issue Oct 1, 2020 · 13 comments
Open

os: handle relative paths in fixLongPath on Windows #41734

rsc opened this issue Oct 1, 2020 · 13 comments
Labels
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Oct 1, 2020

The os package has a function fixLongPath that is used on Windows to turn a very long name like c:\very\long\name.txt to \\?\c:\very\long\name.txt.

This function bails out on relative path names. It also bails out on paths containing ...

This has caused no end of problems and confusion with certain APIs that do or don't accept certain paths.

We should fix fixLongPath to correctly handle all possible inputs and eliminate all those problems.

@networkimprov
Copy link

@networkimprov networkimprov commented Oct 1, 2020

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 19, 2020

Change https://golang.org/cl/263538 mentions this issue: os: on Windows, implement fixLongPath also for relative paths

@networkimprov
Copy link

@networkimprov networkimprov commented Oct 19, 2020

@rasky I think fixLongPath() will need to return an error since syscall.FullPath() can.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 11, 2021

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

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 11, 2021

I've just submitted a patch that avoids the need for any long path nonsense on Windows 10: https://golang.org/cl/291291

That's not as good as a solution that works on all Windows, but we'll eventually drop support for old Windows anyway. So consider this a forward-looking improvement.

cc @jstarks

@jstarks
Copy link

@jstarks jstarks commented Feb 11, 2021

@zx2c4, this is quite clever, but I have some reservations about force-setting the bit--Windows only sets the bit for a process if both the system has long paths enabled and the application statically opts in via manifest (ugh). Replacing this policy (which is process-global, of course) with your own for all Go processes might cause problems. Or it may work just fine!

A safer approach for now would be to query the bit. This would rely on Go developers including a manifest in their binary, though, and most won't. Not great.

I will follow up with the team who built this mechanism to see if they have any guidance.

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 11, 2021

Many Go users build for Windows on Linux, etc, so don't have easy access to a Windows manifest tool. Maybe Go needs a Windows manifest tool or go-build option?

@jstarks
Copy link

@jstarks jstarks commented Feb 11, 2021

See also #17835.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 11, 2021

@zx2c4, this is quite clever, but I have some reservations about force-setting the bit--Windows only sets the bit for a process if both the system has long paths enabled and the application statically opts in via manifest (ugh). Replacing this policy (which is process-global, of course) with your own for all Go processes might cause problems. Or it may work just fine!

I'm pretty sure it would work fine. Unlike random win32 programs, the Go runtime actually controls what's going on, and we're not worried about MAX_PATH buffer overflows or whatever. And if there's a place where it doesn't work fine, we're in a position for it to work fine.

And since the "policy" is per process, Go is in a good place to change it. Currently the thing setting it is just good old kernel32.dll at initialization time (actually kernelbase.dll), nothing fancy.

A safer approach for now would be to query the bit. This would rely on Go developers including a manifest in their binary, though, and most won't. Not great.

I'm not a huge fan of the manifest approach. And I don't like the idea of us twiddling with the registry or even having to only have this if the registry key is enabled. The Go runtime owns its process, so we can very safely just unconditionally enable it.

I will follow up with the team who built this mechanism to see if they have any guidance.

Thanks. Please do relay any interesting concerns that the team that owns this code has. Maybe there's something I don't know about that will come up.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 11, 2021

I don't know anything about this, but what happens if a -buildmode=c-archive Go archive is linked into a C program?

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 11, 2021

Then the resultant C program will handle long paths! IMO, that's a good thing, and a side effect that we can document. If people are writing new software with new Go they're in a place to do things well. OTOH, if you disagree, we could enable this automatically only for exe/pie mode, and put it behind some sort of flag for c-archive/c-shared.

@networkimprov
Copy link

@networkimprov networkimprov commented Feb 20, 2021

See also #44466.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 20, 2021

See also #44466.

I do not believe that issue is very relevant to this issue, because enabling long paths in a manifest file still requires that a global registry knob be turned, which is not something we can rely on happening, and wouldn't want to induce.

On the other hand, https://golang.org/cl/291291 enables the feature per-process, with no global affect, which sounds much more like exactly what we want.

gopherbot pushed a commit that referenced this issue Mar 23, 2021
Windows 10 >= 1607 allows CreateFile and friends to use long paths if
bit 0x80 of the PEB's BitField member is set.

In time this means we'll be able to entirely drop our long path hacks,
which have never really worked right (see bugs below). Until that point,
we'll simply have things working well on recent Windows.

Updates #41734.
Updates #21782.
Updates #36375.

Change-Id: I765de6ea4859dd4e4b8ca80af7f337994734118e
Reviewed-on: https://go-review.googlesource.com/c/go/+/291291
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants