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

syscall: ReadLink() & Utimes() arguments for access-mode & share-mode are wrong #35150

Open
networkimprov opened this issue Oct 25, 2019 · 18 comments

Comments

@networkimprov
Copy link

@networkimprov networkimprov commented Oct 25, 2019

A search for CreateFile() finds the following read-only functions with a zero share-mode argument. I think their share-mode should be FILE_SHARE_READ, and in some cases also FILE_SHARE_WRITE.

EDIT: They would also need FILE_SHARE_DELETE in apps that expect to rename or remove a file concurrently with any of these ops.

readlink()
os/file_windows.go:   h, err := syscall.CreateFile(p, 0, 0, nil,

stat()
os/stat_windows.go:   h, err := syscall.CreateFile(namep, 0, 0, nil,

loadFileId()
os/types_windows.go:  h, err := syscall.CreateFile(pathp, 0, 0, nil,

ReadLink()
syscall/syscall_windows.go: fd, err := CreateFile(StringToUTF16Ptr(path), GENERIC_READ, 0

EDIT: these should probably get | FILE_SHARE_READ

Utimes()
syscall/syscall_windows.go: h, e := CreateFile(pathp, FILE_WRITE_ATTRIBUTES,
                                               FILE_SHARE_WRITE

UtimesNano()
syscall/syscall_windows.go: h, e := CreateFile(pathp, FILE_WRITE_ATTRIBUTES,
                                               FILE_SHARE_WRITE

This simple patch adds FILE_SHARE_DELETE in syscall.Open(), but it doesn't help the above cases:

diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index de05840..e1455d5 100644
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -245,6 +245,8 @@ func makeInheritSa() *SecurityAttributes {
 	return &sa
 }
 
+var Open_FileShareDelete = false
+
 func Open(path string, mode int, perm uint32) (fd Handle, err error) {
 	if len(path) == 0 {
 		return InvalidHandle, ERROR_FILE_NOT_FOUND
@@ -270,6 +272,9 @@ func Open(path string, mode int, perm uint32) (fd Handle, err error) {
 		access |= FILE_APPEND_DATA
 	}
 	sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE)
+	if Open_FileShareDelete {
+		sharemode |= FILE_SHARE_DELETE
+	}
 	var sa *SecurityAttributes
 	if mode&O_CLOEXEC == 0 {
 		sa = makeInheritSa()

cc @alexbrainman @zx2c4 @mattn
@gopherbot add OS-Windows

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Oct 25, 2019

PoC reproducer?

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Oct 25, 2019

This works on my Win7 box.

EDIT: this fails with ERROR_SHARING_VIOLATION in os.Open() on a Win10 box.

EDIT: the failure is reported in #35219. It could be unrelated to the CreateFile() share-mode issue.

package main

import ( "os"; "io/ioutil" )

func main() {
   aFile := "z35150.txt"
   err := ioutil.WriteFile(aFile, []byte("abcdefghijklmnopqrstuvwxyz"), 0600)
   if err != nil { panic(err) }
   go func() {
      for {
         _, err := os.Stat(aFile)
         if err != nil { panic(err) }
      }
   }()
   for {
      _, err := ioutil.ReadFile(aFile)
      if err != nil { panic(err) }
   }
}
@mattn
Copy link
Member

@mattn mattn commented Oct 25, 2019

Because the code is doing that in same process.

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Oct 25, 2019

We already proved that file_share_delete is an issue within the same process, and the CreateFile docs are consistent for file_share_*

EDIT: but if the above fails with two processes, that proves my point :-)

https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea

@networkimprov networkimprov changed the title os, syscall: CreateFile() needs FILE_SHARE_READ for read-only ops os, syscall: CreateFile() needs FILE_SHARE_* Oct 27, 2019
@networkimprov
Copy link
Author

@networkimprov networkimprov commented Oct 28, 2019

I filed a bug report re os.Stat() vs os.Open() in Win10: #35219

This issue should consider necessary & optional share-mode arguments for CreateFile() invocations.

@gopherbot remove release-blocker

@jstarks
Copy link

@jstarks jstarks commented Oct 29, 2019

Although this is not documented well (that I can find), Windows will ignore the share flags if the requested access does not include FILE_READ_DATA, FILE_WRITE_DATA, FILE_EXECUTE, or DELETE. The only case that you've pointed out above is the one in ReadLink, which specifies GENERIC_READ (which is translated internally to FILE_GENERIC_READ, which includes FILE_READ_DATA).

And to fix ReadLink, you can probably just remove GENERIC_READ and replace it with 0 or READ_FILE_ATTRIBUTES--according to my reading of the Windows code, issuing FSCTL_GET_REPARSE_POINT does not require any special permissions on the file. And indeed, internally we generally pass 0 or READ_FILE_ATTRIBUTES when opening files for the purpose of issuing this FSCTL.

Having said all that, adding these share flags in these cases certainly doesn't hurt either--it just isn't necessary to fix any functional bug.

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Oct 29, 2019

Thanks for the drill down! Are you certain that concurrent rename/delete works in the above cases without FILE_SHARE_DELETE?

@jstarks
Copy link

@jstarks jstarks commented Oct 29, 2019

Except for ReadLink, they should work, yes.

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Oct 30, 2019

@zx2c4 wanna patch the access-mode for syscall.ReadLink()?

And drop FILE_SHARE_WRITE in syscall.Utimes/Nano() for consistency.

Then we can close this...

@rsc
Copy link
Contributor

@rsc rsc commented Nov 6, 2019

@alexbrainman and @mattn, do you think we should just hard-code FILE_SHARE_DELETE in these ephemeral, non-os.Open uses of syscall.CreateFile?

@networkimprov networkimprov changed the title os, syscall: CreateFile() needs FILE_SHARE_* syscall: ReadLink() & Utimes() arguments for access-mode & share-mode are wrong Nov 6, 2019
@networkimprov
Copy link
Author

@networkimprov networkimprov commented Nov 6, 2019

Sorry, I should have changed the title to be about syscall.ReadLink() & .Utimes()

FILE_SHARE_DELETE is not at issue here.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 7, 2019

@alexbrainman and @mattn, do you think we should just hard-code FILE_SHARE_DELETE in these ephemeral, non-os.Open uses of syscall.CreateFile?

What is the problem? How do I reproduce the problem?

Thank you.

Alex

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Nov 7, 2019

@alexbrainman two things:

  1. syscall.ReadLink() access-mode argument should be 0 or READ_FILE_ATTRIBUTES (not GENERIC_READ), per #35150 (comment).
  2. syscall.Utimes/UtimesNano() share-mode argument should be 0 (not FILE_SHARE_WRITE).
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 14, 2019

  1. syscall.ReadLink() access-mode argument should be 0 or READ_FILE_ATTRIBUTES (not GENERIC_READ), per #35150 (comment).

I still do not see any problem here.

Sorry.

Alex

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Nov 14, 2019

Alex, did you see #35150 (comment)?

@mattn could you help here?

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 14, 2019

Alex, did you see #35150 (comment)?

Yes, I did.

Alex

@mattn
Copy link
Member

@mattn mattn commented Nov 14, 2019

@networkimprov I don't still understand why you point with example code using FILE_SHARE_DELETE.

#35150 (comment) is related on this issue?

Anyway, AFAIK, eventhough using FILE_SHARE_DELETE, CreateFile possibly be fail when another thread start transaction.

https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfileattributesa#transacted-operations

@networkimprov
Copy link
Author

@networkimprov networkimprov commented Nov 14, 2019

@mattn these are the issues:

  1. syscall.ReadLink() access-mode argument should be 0 or READ_FILE_ATTRIBUTES (not GENERIC_READ), according to @jstarks #35150 (comment).
  2. syscall.Utimes/UtimesNano() share-mode argument should be 0 (not FILE_SHARE_WRITE).

The other posts aren't important. I was seeing a sharing error (due to a OneDrive bug), and thought it might be caused by CreateFile() missing file_share_delete arguments, but it wasn't.

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
8 participants
You can’t perform that action at this time.