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: IsDir doesn't work with Windows OneDrive #22579

Closed
ttrunck opened this issue Nov 4, 2017 · 43 comments
Closed

os: IsDir doesn't work with Windows OneDrive #22579

ttrunck opened this issue Nov 4, 2017 · 43 comments

Comments

@ttrunck
Copy link

@ttrunck ttrunck commented Nov 4, 2017

I think there is an issue with FileInfo.IsDir in Windows 10 using the new Files On-Demand feature of OneDrive. (This occurs only after the Fall Creator Update that introduce this feature).

If I create a folder in OneDrive (with the Files-On-Demand feature on) IsDir return false. Note that just after the creation it return true but once OneDrive finish its sync it then return false. Also if I deactivate the Files-On-Demand feature I no longer repro (for newly created folder, the old one still have the issue). Using the "Always keep on the device option" or the "Free up space" doesn't change the repro.

Also I may miss something obvious, I never used Go before but I found this issue while debugging and issue with Hugo (a site generator in Go).

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

go version go1.9.2 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\theo\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

My OneDrive version is: Version 2017 (Build 17.3.7105.1024)

What did you do?

package main

import (
	"fmt"
	"os"
)

func main() {
	src := "C:\\Users\\theo\\folder"
	srcO := "C:\\Users\\theo\\OneDrive\\folder"
	fi, err := os.Stat(src)
	fmt.Println(fi.IsDir())
	fmt.Println(err)

	fiO, errO := os.Stat(srcO)
	fmt.Println(fiO.IsDir())
	fmt.Println(errO)
}

What did you expect to see?

true
<nil>
true
<nil>

What did you see instead?

true
<nil>
**false**
<nil>
@bradfitz bradfitz changed the title IsDir doesn't work with OneDrive os: IsDir doesn't work with Windows OneDrive Nov 4, 2017
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Nov 4, 2017

What does fi0.Mode().String() print?

@bradfitz bradfitz added this to the Go1.11 milestone Nov 4, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 4, 2017

@ttrunck

This comment has been minimized.

Copy link
Author

@ttrunck ttrunck commented Nov 4, 2017

Thanks for the quick answers.
fmt.Println(fiO.Mode().String()) returns Lrw-rw-rw-

It seems that the issue is here:

if fs.sys.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT != 0 {

I'm not sure why OneDrive folders became symlinks so maybe that's not a bug. But this whole behavior breaks assumptions in other programs so that's kind of an issue.

@ttrunck

This comment has been minimized.

Copy link
Author

@ttrunck ttrunck commented Nov 4, 2017

Or this condition shouldn't be a return but just a m |= ModeSymlink.

But in that case that contradict this old fix (1989921#diff-befffd222e4e39277f81cd4cb7116a0b).

The fact that OneDrive transform stuff in symlinks will probably cause lots of weird issues.

@ssylvan

This comment has been minimized.

Copy link

@ssylvan ssylvan commented Nov 6, 2017

IMHO the ideal situation is that the standard library treats symlinks as transparently as possible, because otherwise many apps doing IO will have to write special code in order to work correctly with symlinks (but since this is subtle, it would probably mean that in practice apps written in Go wouldn't work correctly with symlinks).

While the change introducing this issue was in response to some valid bugs, it would seem preferable that the specific cases where symlinks should not be handled transparently should explicitly check for it (e.g. when doing recursive traversal).

@NexWeb

This comment has been minimized.

Copy link

@NexWeb NexWeb commented Nov 7, 2017

With the same configuration, go version go1.9.2 windows/amd64, os.Stat()
returns Directory = true, however Lstat() correctly identifies the symbolic link directory.
File name: folderSymlink
Size in bytes: 0
Permissions: Lrw-rw-rw-
Is Directory: false

IMO, this is appropriate behavior because certain considerations need to be addressed and it provides the opportunity to specify, among other things a FILE_FLAG_OPEN_REPARSE_POINT flag.

Symbolic links, as reparse points, have certain programming considerations specific to them.

  • Symbolic links can point to a non-existent target.
  • When creating a symbolic link, the operating system does not check to see if the target exists.
  • If an application tries to open a non-existent target, ERROR_FILE_NOT_FOUND is returned.
  • Symbolic links are reparse points.
  • There is a maximum of 31 reparse points (and therefore symbolic links) allowed in a particular path.

The OpenFileById function will either open the file or the reparse point, depending on the use of the FILE_FLAG_OPEN_REPARSE_POINT flag.

Backup applications that use file streams should specify BACKUP_REPARSE_DATA in the WIN32_STREAM_ID structure when backing up files with reparse points.
Applications that use the CreateFile function should specify the FILE_FLAG_OPEN_REPARSE_POINT flag when opening the file, if it is a reparse point.

Also, consider the following information regarding FILE_FLAG_OPEN_REPARSE_POINT:
If FILE_FLAG_OPEN_REPARSE_POINT is specified:
If an existing file is opened and it is a symbolic link, the handle returned is a handle to the symbolic link.
If TRUNCATE_EXISTING or FILE_FLAG_DELETE_ON_CLOSE are specified, the file affected is a symbolic link.
If FILE_FLAG_OPEN_REPARSE_POINT is not specified:
If an existing file is opened and it is a symbolic link, the handle returned is a handle to the target.
If CREATE_ALWAYS, TRUNCATE_EXISTING, or FILE_FLAG_DELETE_ON_CLOSE are specified, the file affected is the target.

var fileInfo os.FileInfo
func main() {
	src := "C:\\Users\\nexweb\\go\\src"
	srcO := "C:\\Users\\nexweb\\OneDrive\\folderSymlink"
	fi, err := os.Stat(src)
	switch mode := fi.Mode(); {
	case mode.IsRegular():
		fmt.Println("regular file")
	case mode.IsDir():
		fmt.Println("directory")
	case mode&os.ModeSymlink != 0:
		fmt.Println("symbolic link")
	case mode&os.ModeNamedPipe != 0:
		fmt.Println("named pipe")
	}
	fmt.Println(err)
	fmt.Println(fi.Mode().String())

	fiO, errO := os.Stat(srcO)
	switch mode := fiO.Mode(); {
	case mode.IsRegular():
		fmt.Println("regular file")
	case mode.IsDir():
		fmt.Println("directory")
	case mode&os.ModeSymlink != 0:
		fmt.Println("symbolic link")
	case mode&os.ModeNamedPipe != 0:
		fmt.Println("named pipe")
	}
	fmt.Println(errO)
	fmt.Println(fiO.Mode().String())
	fileInfo, err = os.Lstat(srcO)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println("File name:", fileInfo.Name())
	fmt.Println("Size in bytes:", fileInfo.Size())
	fmt.Println("Permissions:", fileInfo.Mode())
	fmt.Println("Last modified:", fileInfo.ModTime())
	fmt.Println("Is Directory: ", fileInfo.IsDir())
}

@matthijskooijman

This comment has been minimized.

Copy link
Contributor

@matthijskooijman matthijskooijman commented Nov 16, 2017

Users of the Arduino IDE are also reporting problems with OneDrive files not being read by the arduino-builder tool, which is written in go. We're tracking the issue at arduino/arduino-builder#254 The problem there is ReadLink failing, which is (probably) different from this issue, but I think the underlying cause might be the same.

I'm not sure why OneDrive folders became symlinks so maybe that's not a bug.

I believe the OneDrive files/folders are not symlinks, but they are "reparse points" (and the most common types of reparse points seem to be symlinks and junctions, both kinds of links that ReadLink now supports explicitely). This post suggests they are "Cloud reparse points", though I'm not sure what that means exactly.

Perhaps this provides some starting points for fixing this (and perhaps also ReadLink)?

@ssylvan

This comment has been minimized.

Copy link

@ssylvan ssylvan commented Nov 16, 2017

Interesting.. If they are not true symlinks, then it would seem that would strengthen the case that the IO library should treat them as files transparently. Especially if there are guarantees that "cloud reparse points" will always behave just as files do (in the sense that they're not going to cause loops while listing directory contents, etc.).

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 16, 2017

Users of the Arduino IDE are also reporting problems with OneDrive files not being read by the arduino-builder tool, which is written in go. We're tracking the issue at arduino/arduino-builder#254 The problem there is ReadLink failing, which is (probably) different from this issue, but I think the underlying cause might be the same.

@matthijskooijman

Looking at your bug arduino/arduino-builder#254

readlink C:\Users\HaMac\OneDrive\Documents\Ardunio\BlinkTwoLEDs\BlinkTwoLEDs.ino: The system cannot find the file specified.

I would agree that it, probably, is different problem. This current issue is about os IsDir. While yours is os.Readlink. They might be related, but they different problems.

I suggest you file a new bug with small reproducible example. Mind you, os.Readlink is broken on Windows - there is no way to implement it properly on Windows. I suggest you use filepath.EvalSymlinks instead. Would that work for you?

Alex

PS: I still do not have Windows with "Fall Creator Update" to debug this.

@matthijskooijman

This comment has been minimized.

Copy link
Contributor

@matthijskooijman matthijskooijman commented Nov 17, 2017

I suggest you file a new bug with small reproducible example.

Yeah, I was hoping to do that, but no time at all right now. I was mostly thinking that the info would be useful for this bug as well.

Mind you, os.Readlink is broken on Windows - there is no way to implement it properly on Windows. I suggest you use filepath.EvalSymlinks instead. Would that work for you?

IIRC we're using Filepath.Walk which apparently uses ReadLink internally. I'm not 100% sure yet if thats's also the case here, but it was in a previous bug with the same symptom.

PS: I still do not have Windows with "Fall Creator Update" to debug this.

Me neither, I'm just basing on reports by our users.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 18, 2017

IIRC we're using Filepath.Walk which apparently uses ReadLink internally.

I don't see that filepath.Walk uses os.Readlink. But I have been wrong before. That is why I suggest you create a new issue with small reproducible example, if you have one.

Alex

@matthijskooijman

This comment has been minimized.

Copy link
Contributor

@matthijskooijman matthijskooijman commented Nov 18, 2017

I don't see that filepath.Walk uses os.Readlink.

I've also looked, and couldn't find this either (perhaps it was a wrong conclusion before). I couldn't actually find any path to os.Readlink, so I'm not quite sure what happens yet exactly, so haven't been able to create a small example yet.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 7, 2017

@ttrunck I finally managed to update my Windows 10 to correct version, and I can reproduce something similar to your bug.

I suspect the problem is that we don't follow advise from https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/ to the letter. In particular, it says: "... If you use the FindFirstFile function, you can tell that you have a symbolic link because the file attributes will have the FILE_ATTRIBUTES_REPARSE_POINT flag set, and the dwReserved0 member will contain the special value IO_REPARSE_TAG_SYMLINK. ...". We do check the FILE_ATTRIBUTES_REPARSE_POINT flag set, but we do not check dwReserved0. We should rectify this problem, but only once go1.10 is released, because I suspect the change might be involved. We need to fix everything: os.Stat and os.File.Stat and os.Lstat and os.Readdir.

Alex

@ttrunck

This comment has been minimized.

Copy link
Author

@ttrunck ttrunck commented Dec 7, 2017

Thanks for looking at it!
So it's more involved than just not breaking early in types_windows.Mode

So this should be fixed in go1.11 and the timeline is mid 2018 right? For now my workaround is to not use that OneDrive feature or move my projects outside of OneDrive. I also think Docker is hitting this issue but I don't know how I can easily confirm that it's indeed the same issue.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 7, 2017

I punted this to Go 1.11 because old bugs (ones that aren't recent regressions) always get punted. But if this is suddenly more prominent because of OneDrive, we could consider a fix for Go 1.10, if somebody could prepare one soon that checks dwReserved0.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 7, 2018

Change https://golang.org/cl/86556 mentions this issue: os: use WIN32_FIND_DATA.Reserved0 to identify symlinks

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 7, 2018

So this should be fixed in go1.11 and the timeline is mid 2018 right?

Yes, the fix is bit convoluted, so it might have to wait for go1.11. @ttrunck can you, please, try the fix here https://golang.org/cl/86556 ? Does it fixes your problem? Thank you.

Alex

@ttrunck

This comment has been minimized.

Copy link
Author

@ttrunck ttrunck commented Jan 7, 2018

Hi Alex,

Thanks for your work. I will find some time this week to check if this fixes my problem.

To be sure I have to build both Go and Hugo (the original program where I noticed the issue) right? The library isn't dynamically linked?

As I mentioned I'm just a user of a program written in Go and have zero experience with this language, so my check shouldn't worth more than "I found a case not working and with your patch it's now fixed"

Thanks

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 7, 2018

To be sure I have to build both Go and Hugo (the original program where I noticed the issue) right? The library isn't dynamically linked?

Go code is statically linked into the binary if you use the default Go compiler, which includes the standard library such as in this case.

Pulling in the potential fix to the standard library might be a bit more confusing if you're not used to building Go from source. I would suggest looking at https://golang.org/doc/install/source. You could do something like:

  • clone Go repository (getting the master tree)
  • apply the commit referenced above
  • build with ./make.bash
  • use this built Go to then build Hugo (the doc linked above should help you here)
@ttrunck

This comment has been minimized.

Copy link
Author

@ttrunck ttrunck commented Jan 7, 2018

Perfect I should be good to go. Thanks for your help.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 8, 2018

I am expanding on some @mvdan instructions ...

clone Go repository (getting the master tree)

From https://golang.org/doc/install/source#fetch

git clone https://go.googlesource.com/go

apply the commit referenced above

Go to https://golang.org/cl/86556 click on Download in the right top corner and copy Checkout instructions into clipboard and run them

git fetch https://go.googlesource.com/go refs/changes/56/86556/1 && git checkout FETCH_HEAD

build with ./make.bash

cd %GOROOT%\src
make.bat

if you are on Windows.

Alex

@ttrunck

This comment has been minimized.

Copy link
Author

@ttrunck ttrunck commented Jan 16, 2018

Hi Alex,

It took me a bit longer that expected to build/check your fix. CGO_ENABLED need to be kept to 1 but since minGW only give a 32 bits gcc, the easiest workaround is to set GOHOSTARCH to 386. From what I understand it shouldn't be an issue.

So go version gave me

go version devel +545ea9ddb7 Sun Jan 7 19:26:46 2018 +1100 windows/386

As you probably know my exemple from the first post is now working as expected.

But Hugo is still not working. If I use the integrated server, (hugo.exe server) some files aren't served.
And if I try to build I got this stack trace:

?[?25lBuilding sites … 2018/01/15 22:21:12 mkdir C:\Users\ttrun\OneDrive\Projets\home\public\post\post1\: Cannot create a file when that file already exists.
2018/01/15 22:21:12 mkdir C:\Users\ttrun\OneDrive\Projets\home\public\post\post2\: Cannot create a file when that file already exists.
panic: mkdir C:\Users\ttrun\OneDrive\Projets\home\public\post\post1\: Cannot create a file when that file already exists.


goroutine 120 [running]:
log.Panicln(0x1500dc3c, 0x1, 0x1)
        C:/Users/ttrun/Desktop/Go/src/log/log.go:340 +0x9d
github.com/spf13/afero.WriteReader(0x190de00, 0x1ccd3c0, 0x14de6540, 0x53, 0x1901160, 0x14f30480, 0x1901f60, 0x150b13e0)
        C:/Users/ttrun/go/src/github.com/spf13/afero/util.go:49 +0x1bb
github.com/gohugoio/hugo/helpers.WriteToDisk(0x14de6540, 0x53, 0x1901160, 0x14f30480, 0x190de00, 0x1ccd3c0, 0x4280e0, 0x4)
        C:/Users/ttrun/go/src/github.com/gohugoio/hugo/helpers/path.go:532 +0x49
github.com/gohugoio/hugo/hugolib.(*Site).publish(0x14e7f0e0, 0x14fb3188, 0x14dde6c0, 0x28, 0x1901160, 0x14f30480, 0x0, 0x0)
        C:/Users/ttrun/go/src/github.com/gohugoio/hugo/hugolib/site.go:1792 +0xe0
github.com/gohugoio/hugo/hugolib.(*Site).renderAndWritePage(0x14e7f0e0, 0x14fb3188, 0x14dde6f0, 0x24, 0x14dde6c0, 0x28, 0x153e0000, 0x15192100, 0x10, 0x10, ...)
        C:/Users/ttrun/go/src/github.com/gohugoio/hugo/hugolib/site.go:1731 +0x3f9
github.com/gohugoio/hugo/hugolib.pageRenderer(0x14e7f0e0, 0x14e1c1c0, 0x14e1c180, 0x15002580)
        C:/Users/ttrun/go/src/github.com/gohugoio/hugo/hugolib/site_render.go:148 +0x5aa
created by github.com/gohugoio/hugo/hugolib.(*Site).renderPages
        C:/Users/ttrun/go/src/github.com/gohugoio/hugo/hugolib/site_render.go:45 +0x110

I also built Hugo from a not patched Go and if I build I still got my previous error:

?[KTotal in 9 msites … ?[?25h
Error: Error copying static files: open C:\Users\ttrun\OneDrive\Projets\home\public\: is a directory

Finally if I use the Hugo.exe build from the patched Go in a directory outside of Onedrive it's working as expected.

I haven't dig into the issue yet and I'm not sure that the stack trace will be helpful. I will try to debug that issue next week-end and provide a good minimal exemple.

Thanks

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 16, 2018

CGO_ENABLED need to be kept to 1 but since minGW only give a 32 bits gcc

You should not need minGW to test my changes. If you struggle with minGW, just set CGO_ENABLED=0.

the easiest workaround is to set GOHOSTARCH to 386. From what I understand it shouldn't be an issue.

386 should work too, but with CGO_ENABLED=0 you could build things without fiddling with GOHOSTARCH.

So go version gave me

go version devel +545ea9ddb7 Sun Jan 7 19:26:46 2018 +1100 windows/386

545ea9ddb7 commit looks good - I have the same.

But Hugo is still not working.

Well I need instructions on how to reproduce your problem, because my CL 86556 fixes problem you described here #22579 (comment) Maybe your Hugo problem is different problem. It could be even bug in Hugo.

Finally if I use the Hugo.exe build from the patched Go in a directory outside of Onedrive it's working as expected.

That is good to know - my CL 86556 did not introduce any new problems for your non-OneDrive environment.

I haven't dig into the issue yet and I'm not sure that the stack trace will be helpful.

Yes, I would need an instructions on how reproduce that (at the very least). And I do not have much free time this days, so the simpler your reproduction the better for me.

I will try to debug that issue next week-end and provide a good minimal exemple.

That would be good. Thank you.

Alex

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 23, 2018

Change https://golang.org/cl/108755 mentions this issue: os: check error ERROR_INVALID_NAME from GetFileAttributeEx in os.Stat("*.txt")

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 23, 2018

Change https://golang.org/cl/108776 mentions this issue: os: check error ERROR_INVALID_NAME from GetFileAttributeEx in os.Stat("*.txt")

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 8, 2018

But I agree, perhaps, we should use IsReparseTagNameSurrogate to decide if file is symlink or not.

@alexbrainman So, basically, something like this:

diff --git a/src/os/types_windows.go b/src/os/types_windows.go
index f3297c0338..6668d911fc 100644
--- a/src/os/types_windows.go
+++ b/src/os/types_windows.go
@@ -140,14 +140,14 @@ func (fs *fileStat) updatePathAndName(name string) error {
 }
 
 func (fs *fileStat) isSymlink() bool {
+       const reparsePointNameSurrogate = 0x20000000
        // Use instructions described at
        // https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
        // to recognize whether it's a symlink.
        if fs.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT == 0 {
                return false
        }
-       return fs.Reserved0 == syscall.IO_REPARSE_TAG_SYMLINK ||
-               fs.Reserved0 == windows.IO_REPARSE_TAG_MOUNT_POINT
+       return fs.Reserved0&reparsePointNameSurrogate != 0
 }
 
 func (fs *fileStat) Size() int64 {

would be good to have. Let me know if you want me to open a CI.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 9, 2018

@alexbrainman So, basically, something like this:

Yes. Perhaps more comments are needed to explain the 0x20000000.

would be good to have.

I don't see anything wrong with existing code - the code has comment explaining why it does what it does. What do you think the code needs updating? Do you have an issue that is affected by this code?

Alex

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 9, 2018

What do you think the code needs updating?

@alexbrainman per your own earlier comment: #22579 (comment) (also see comments above that one).

Do you have an issue that is affected by this code?

We had an issue with Docker on Windows with deduplication feature turned on (moby/moby#37026) which was solved (in moby/moby#37769) in a way very similar to your commit e83601b, but with the final check for is IsReparseTagNameSurrogate (i.e. same as in my example patch above).

Perhaps both ways to check are equally correct for now, but it seems that the check for IsReparseTagNameSurrogate might be more future-proof.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 13, 2018

We had an issue with Docker on Windows with deduplication feature turned on (moby/moby#37026) which was solved (in moby/moby#37769) in a way very similar to your commit e83601b, but with the final check for is IsReparseTagNameSurrogate (i.e. same as in my example patch above).

Sounds reasonable. But, please, create new issue first at https://golang.org/issues/new with simple repro so we have some history about why we made the change. Please, also describe the environment needed to be able to reproduce this issue.

Thank you.

Alex

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 13, 2018

@kolyshkin also I recently fixed os.SameFile - it was completely broken when os.SameFile parameters were symlinks - 1c95d97 . Maybe it will fix your issue.

Alex

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 21, 2018

Perhaps both ways to check are equally correct for now, but it seems that the check for IsReparseTagNameSurrogate might be more future-proof.

@kolyshkin, I decided to get rid of that code altogether. Here is my proposal https://go-review.googlesource.com/c/go/+/143578

Alex

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 22, 2018

@alexbrainman it looks like you oversimplified things too much (maybe because you assumed that FILE_ATTRIBUTE_REPARSE_POINT can be set only for either symlinks or directories -- in addition to these two cases I know it can also be set on files residing on volumes with data deduplication turned on, see moby/moby#37769 and also "steps to reproduce" from moby/moby#37026). With this in mind, the check

func (fs *fileStat) isSymlink() bool {
return fs.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT != 0

is not sufficient, you'll get false positives (isSymlink() returning true for file systems objects that are not "symlinks".

The correct check should look something like

func (fs *fileStat) isSymlink() bool {
	const reparsePointNameSurrogate = 0x20000000 // TODO document this
	return fs.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT != 0 &&
		fs.Reserved0&reparsePointNameSurrogate != 0
}

Of course, Reserved0 should be populated from syscall.FindFirstFile by newFileStatFromWin32finddata, which your patch removed.

PS all this is theorizing based on the code I've seen; I don't even have a Windows system to try things out.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 24, 2018

it can also be set on files residing on volumes with data deduplication turned on, see moby/moby#37769 and also "steps to reproduce" from moby/moby#37026)

Both moby/moby#37769 and moby/moby#37026 describe repro for a bug in Docker. I do not see how that bug is relevant to my change in https://go-review.googlesource.com/c/go/+/143578 . I have never looked at Docker code, and I do not have time to debug Docker. If you can distill that bug into small Go program, I will try and investigate it.

As to your "theorizing", maybe you are correct, maybe you are wrong. I am not aware of any issues that https://go-review.googlesource.com/c/go/+/143578 does not cover. So I am going to proceed with my change, until I see something that breaks with my change.

Alex

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 24, 2018

Both moby/moby#37769 and moby/moby#37026 describe repro for a bug in Docker

@alexbrainman ... which is caused merely by incorrect identification of symlinks on Windows (which is fixed in Go 1.11 by your commit e83601b). I'm afraid it's going to be broken again.

You don't have to dig into Docker source code, or to use Docker in order to reproduce. If you can take another look at moby/moby#37769 description, in particular "How to verify" section, you'll find the steps to enable deduplication. Once enabled, the test to perform (without using Docker) is to iterate through the files, doing something like this for each one:

	fi, err := os.Lstat(path)
	if err != nil {
		return err
	}
 
 	if fi.Mode()&os.ModeSymlink != 0 {
 		link, err := os.Readlink(path)
 		if err != nil {
 			return err
 		}
 	}

As you can see, the code assumes if a file is a symlink, os.Readlink() can be used.

Previously (before commit e83601b) the above code failed on deduplicated files with the error like this:

readlink \?\D:\temp\file: The system cannot find the file specified.

From what I see, this bug might come back with the change in https://go-review.googlesource.com/c/go/+/143578. Ideally, of course, there should be a unit test added to check for this specific issue (treating dedup'ed files like symlinks).

I apologize for not making things clear earlier, and hope this explanation makes sense. This is as close as I can get to presenting a simple reproducer without actual access to a Windows machine.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 25, 2018

@alexbrainman ... which is caused merely by incorrect identification of symlinks on Windows (which is fixed in Go 1.11 by your commit e83601b). I'm afraid it's going to be broken again.

Maybe yes, maybe not. We won't know that until we apply CL 143578 to your test case. If you give me your test case, I will try it, if I can. If you cannot provide me with a test case, I cannot even try. Feel free to try CL 143578 yourself, and report results back here. I will happy to help with instructions, if you need any.

iterate through the files, doing something like this for each one:

This is to vague for a test. Please use https://github.com/golang/go/issues/new and answer all questions there.

os.Readlink() can be used.

os.Readlink is broken on Windows in many situations. You should not be using it in your code.

Ideally, of course, there should be a unit test added to check for this specific issue (treating dedup'ed files like symlinks).

I agree, it might be difficult to have permanent test. But I would like to try and reproduce it myself at least somewhere.

This is as close as I can get to presenting a simple reproducer without actual access to a Windows machine.

Unfortunately this is not enough for me for a repro.

Maybe you should try and explain what your are trying to do? Perhaps I might suggest the code that will work for you.

Alex

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 25, 2018

Maybe you should try and explain what your are trying to do? Perhaps I might suggest the code that will work for you.

The code in question adds a file to a tar; the relevant part is creating a tar entry header. This is the code (
https://github.com/moby/moby/blob/bc4c1c238b551ce58fc26690523354d1571bab1d/pkg/archive/archive.go#L456-L475):

// addTarFile adds to the tar archive a file from `path` as `name`
func (ta *tarAppender) addTarFile(path, name string) error {
	fi, err := os.Lstat(path)
	if err != nil {
		return err
	}

	var link string
	if fi.Mode()&os.ModeSymlink != 0 {
		var err error
		link, err = os.Readlink(path)
		if err != nil {
			return err
		}
	}

	hdr, err := FileInfoHeader(name, fi, link)
	if err != nil {
		return err
	}

On a system with deduplication turned on and before commit e83601b (and I suspect after CL 143578), this leads to

readlink \\?\D:\temp\dedupme\CbsApi.dll: The system cannot find the file specified.

As you can see, the code assumes that if os.ModeSymlink bit is set in s.Mode() returned by os.Lstat(), then os.Readlink() can be used. Is this assumption wrong?

You say that

os.Readlink is broken on Windows in many situations. You should not be using it in your code.

Can you please elaborate (and/or propose a replacement for the above code)?

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 27, 2018

@kolyshkin I played with files on deduped volume. I am happy to report that, you are correct - all files on deduped volume looks like files now, because Windows reports then with both FILE_ATTRIBUTE_REPARSE_POINT and IO_REPARSE_TAG_DEDUP set. So after my CL 143578 - which only checks for FILE_ATTRIBUTE_REPARSE_POINT - they will be interpreted as symlinks. I suppose you are saying they should be plain files, not symlinks, and I cannot go ahead with CL 143578 unless I find way to check for IO_REPARSE_TAG*. Fair enough.

Alex

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 27, 2018

@alexbrainman glad to be of help.

Checking IO_REPARSE_TAG_* is not a good idea though, as it's not future-proof (say MS adds some other fancy feature and a flag, and then your code needs to be updated yet again to account for this new flag).

It still seems to me the correct check is the one described in #22579 (comment), ie the check for reparsePointNameSurrogate (0x20000000) in Reserved0 field, populated from syscall.FindFirstFile by newFileStatFromWin32finddata. This is very similar to what the code in Go 1.11 does. I could create a CL but need to go through the horrors of setting up Windows development environment first

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 27, 2018

It still seems to me the correct check is the one described in #22579 (comment), ie the check for reparsePointNameSurrogate (0x20000000) in Reserved0 field,

One thing at a time. I am trying to fix #27225 and #27515 at this moment.

Alex

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 28, 2018

@kolyshkin

I adjusted CL 143578 for my yesterday's findings. I hope after CL 143578 deduped volumes will work as good / bad as they do now.

Alex

@kolyshkin

This comment has been minimized.

Copy link
Contributor

@kolyshkin kolyshkin commented Oct 30, 2018

@alexbrainman thank you Alex, it looks like CL 143578 (patch set 3) is good, I have left my review comments right in there (should have done it in the first place, sorry for abusing github).

@golang golang locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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