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

networkimprov · 2019-10-25T08:24:52Z

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

The text was updated successfully, but these errors were encountered:

zx2c4 · 2019-10-25T08:32:10Z

PoC reproducer?

networkimprov · 2019-10-25T10:29:28Z

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 · 2019-10-25T10:33:51Z

Because the code is doing that in same process.

networkimprov · 2019-10-25T10:46:02Z

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 · 2019-10-28T20:53:36Z

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 · 2019-10-29T18:57:12Z

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 · 2019-10-29T19:06:48Z

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

jstarks · 2019-10-29T19:15:49Z

Except for ReadLink, they should work, yes.

networkimprov · 2019-10-30T04:26:20Z

@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 · 2019-11-06T18:44:17Z

@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 · 2019-11-06T20:51:02Z

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

FILE_SHARE_DELETE is not at issue here.

alexbrainman · 2019-11-07T08:45:19Z

@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 · 2019-11-07T09:16:53Z

@alexbrainman two things:

syscall.ReadLink() access-mode argument should be 0 or READ_FILE_ATTRIBUTES (not GENERIC_READ), per syscall: ReadLink() & Utimes() arguments for access-mode & share-mode are wrong #35150 (comment).
syscall.Utimes/UtimesNano() share-mode argument should be 0 (not FILE_SHARE_WRITE).

alexbrainman · 2019-11-14T08:50:02Z

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 · 2019-11-14T09:04:02Z

Alex, did you see #35150 (comment)?

@mattn could you help here?

alexbrainman · 2019-11-14T09:40:14Z

Alex, did you see #35150 (comment)?

Yes, I did.

Alex

mattn · 2019-11-14T09:52:27Z

@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 · 2019-11-14T10:25:42Z

@mattn these are the issues:

syscall.ReadLink() access-mode argument should be 0 or READ_FILE_ATTRIBUTES (not GENERIC_READ), according to @jstarks syscall: ReadLink() & Utimes() arguments for access-mode & share-mode are wrong #35150 (comment).
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.

gopherbot added the OS-Windows label Oct 25, 2019

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 25, 2019

gopherbot added the release-blocker label Oct 26, 2019

networkimprov mentioned this issue Oct 26, 2019

proposal: syscall: define Windows O_ALLOW_DELETE for use in os.OpenFile #34681

Closed

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

gopherbot removed the release-blocker label Oct 28, 2019

andybons mentioned this issue Nov 6, 2019

proposal: review meeting minutes #33502

Open

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

Stebalien mentioned this issue Apr 10, 2020

Error writing to datastore on Windows 10 ipfs/kubo#7115

Closed

seankhliao added this to the Unplanned milestone Aug 27, 2022

bcmills mentioned this issue Oct 17, 2022

os: File.Stat on Windows doesn't use file handle for directories #52747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

networkimprov commented Oct 25, 2019 •

edited

zx2c4 commented Oct 25, 2019

networkimprov commented Oct 25, 2019 •

edited

mattn commented Oct 25, 2019

networkimprov commented Oct 25, 2019 •

edited

networkimprov commented Oct 28, 2019

jstarks commented Oct 29, 2019

networkimprov commented Oct 29, 2019

jstarks commented Oct 29, 2019

networkimprov commented Oct 30, 2019

rsc commented Nov 6, 2019

networkimprov commented Nov 6, 2019 •

edited

alexbrainman commented Nov 7, 2019

networkimprov commented Nov 7, 2019

alexbrainman commented Nov 14, 2019

networkimprov commented Nov 14, 2019

alexbrainman commented Nov 14, 2019

mattn commented Nov 14, 2019

networkimprov commented Nov 14, 2019

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

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

Comments

networkimprov commented Oct 25, 2019 • edited

zx2c4 commented Oct 25, 2019

networkimprov commented Oct 25, 2019 • edited

mattn commented Oct 25, 2019

networkimprov commented Oct 25, 2019 • edited

networkimprov commented Oct 28, 2019

jstarks commented Oct 29, 2019

networkimprov commented Oct 29, 2019

jstarks commented Oct 29, 2019

networkimprov commented Oct 30, 2019

rsc commented Nov 6, 2019

networkimprov commented Nov 6, 2019 • edited

alexbrainman commented Nov 7, 2019

networkimprov commented Nov 7, 2019

alexbrainman commented Nov 14, 2019

networkimprov commented Nov 14, 2019

alexbrainman commented Nov 14, 2019

mattn commented Nov 14, 2019

networkimprov commented Nov 14, 2019

networkimprov commented Oct 25, 2019 •

edited

networkimprov commented Oct 25, 2019 •

edited

networkimprov commented Oct 25, 2019 •

edited

networkimprov commented Nov 6, 2019 •

edited