proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows

@rsc

On Linux & MacOS we can write this; on Windows it fails with a "sharing violation":

path := "delete-after-open"
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
if err != nil { ... }
err = os.Remove(path)            // or os.Rename(path, path+"2")
if err != nil { ... }
fd.Close()

If you develop on Windows and deploy to Linux etc, and your code relies on this undocumented GOOS=windows behavior of os.Rename() & .Remove(), it is broken and perhaps vulnerable. Note that package "os" has fifteen mentions of other Windows-specific behavior.

To fix this, syscall.Open() at https://golang.org/src/syscall/syscall_windows.go#L272
needs sharemode |= FILE_SHARE_DELETE

Microsoft recommends this be made the default: #32088 (comment)
Rust made it a default: https://doc.rust-lang.org/std/os/windows/fs/trait.OpenOptionsExt.html
Mingw-w64 made it a default seven years ago:
https://sourceforge.net/p/mingw-w64/code/HEAD/tree/stable/v3.x/mingw-w64-headers/include/ntdef.h#l858
Erlang made it a default over six years ago: erlang/otp@0e02f48
Python couldn't adopt it due to limitations of MSVC runtime: https://bugs.python.org/issue15244

Therefore syscall.Open() should use file_share_delete by default, and syscall should provide both:
a) a global switch to disable it (for any existing apps that rely on its absence), and
b) a flag for use with os.OpenFile() to disable it on a specific file handle.

Update after #32088 (comment) by @rsc:
a) os.Create/Open/OpenFile() should always enable file_share_delete on Windows,
b) syscall.Open() on Windows should accept a flag which enables file_share_delete, and
c) syscall on Windows should export a constant for the new flag.

The docs for os.Remove() should also note that to reuse a filename after deleting an open file on Windows, one must do: os.Rename(path, unique_path); os.Remove(unique_path).

Win API docs
https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-deletefilea
https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-createfilea

If there is a reason not to do this, it should be documented in os.Remove() & .Rename().

cc @alexbrainman
@gopherbot add OS-Windows

@alexbrainman

/cc @alexbrainman

syscall.Open() at https://golang.org/src/syscall/syscall_windows.go#L272
should use sharemode := ... | FILE_SHARE_DELETE

Why should it?

Alex

FILE_SHARE_DELETE enables the code example above, which works on MacOS & Linux but fails on Windows. It's also necessary for:

fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
defer fd.Close()
_, err = fd.Write(...)
err = fd.Sync()  // file now safe to share
err = os.Rename(path, path+"2")
_, err = fd.Read(...)

FILE_SHARE_DELETE enables the code example above, which works on MacOS & Linux but fails on Windows.

Why don't we adjust MacOS & Linux code instead?

It's also necessary for:

I don't understand what you are trying to say.

Alex

@networkimprov

@networkimprov You must call Remove after Close() on Windows.

path := "delete-after-open"
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
if err != nil { panic(err) }
fd.Close()
err = os.Remove(path)            // or os.Rename(path, path+"2")
if err != nil { panic(err) }

@alexbrainman

@alexbrainman when doing reliable file I/O (as for databases), it's standard practice to create a file with a temporary name, write it, fsync it, and rename it.

Open files can be renamed or deleted by default on Unix. It seems like an oversight that the Windows flag for this capability is not set. I doubt we'll convince the Go team to change the way it works for Linux & MacOS :-)

@mattn pls apply the fix I described and try the code I posted.

I doubt we'll convince the Go team to change the way it works for Linux & MacOS :-)

I am fine the way things are now.

Alex

Is there a rationale for omitting this common capability in Windows?

Can you provide a switch in syscall_windows.go so that we can select the Unix behavior at program start?

I'm okay that we add new API or flags. But I have objection to change current behavior. Since FILE_SHARE_DELETE is not same as Unix behavior.

#include <windows.h>
#include <stdio.h>

int
main(int argc, char* argv[]) {
  HANDLE h = CreateFile("test.txt",
      GENERIC_READ | GENERIC_WRITE,
      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
      NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
  getchar();
  char buf[256] = {0};
  DWORD nread;
  printf("%d\n", ReadFile(h, buf, 256, &nread, NULL));
  printf("%s,%d\n", buf, nread);
  return 0;
}

Comple this code on Windows, and try to run as test.exe. While this app is waiting hit-a-key, open new cmd.exe, delete "test.txt" like following.

The file can be deleted but remaining there while the process exists. So this change will not work well for your expected.

I realize the directory entry isn't removed but the docs say

Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

So I don't understand your screen log.

Anyway, a switch like this would be fine:

syscall.OpenFileShareDelete = true

I’d like to mention that I’ve been bitten by this at work before.

Basically we had:

f, err := os.Open(...)
if err != nil { ... }
defer f.Close()

// lots of code

if err := os.Rename(...); err != nil { ... }

The code ran fine on our unix-based CI platforms, but exploded on Windows.

Assuming there aren’t any weird side effects, it would be nice if things just worked.

@mattn

IIRC, we've made a few adjustments to GOOS=windows & plan9 behavior in the past to more closely match Unix semantics. I wouldn't mind making this be another such case if the semantics are close enough. @mattn's comment, however, suggests the behavior is not close enough so it might not be worth it.

I don't want to see some global option, though. That just seems like a debugging nightmare.

I'm really disappointed by this response. Copying this large swath of code, which is largely not even understood (as in why is this doing what it's doing) just so we can add a single option that the system itself supports, just that Go, mostly likely I assume not even intentionally, doesn't provide a means to pass the option down even to syscall.Open seems like a rediculous situation. Why do I have to rewrite syscall.Open to pass this option in?

On Sat, Oct 5, 2019 at 18:43 Alex Brainman ***@***.***> wrote: Alex, one can loop the rename attempt if outside access is allowed, ... If you are prepared to loop the rename, you don't need to change anything in Go repo code. Your program will work as good as it works now. Right now, the only solution is to literally copy a bunch of code out of Go's standard library, change one line, and then maintain that fork forever. I think copying code into separate package is fine. While investigating moby/moby#39974 <moby/moby#39974> I got the same idea. I don't think there is much code there to maintain. In fact I implemented just that https://github.com/alexbrainman/goissue34681 Feel free to copy or use as is. Maybe, given, there is so much interest in this functionality, you actually create proper package that can be shared and used by others. This way you can keep it up to date and fix bugs. @rsc <https://github.com/rsc>, before you decline this, let's hear what Alex thinks of your O_ALLOW_DELETE proposal. Please, try https://github.com/alexbrainman/goissue34681 first. If you not satisfied for some reason, we could discuss adding new functionality to Go repo. Thank you. Alex — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32088?email_source=notifications&email_token=AAGDCZXHULQEMHAPTO6ZUJTQNFGE7A5CNFSM4HNPNYIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAN7QIQ#issuecomment-538703906>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGDCZWYUNMCTAGO567AV73QNFGE7ANCNFSM4HNPNYIA> .

-- - Brian Goff

I'm really disappointed by this response.

I am sorry you feel that way. But did you actually tried to use my package?

Alex

@rsc

@rsc, since Alex is also opposed to an os.OpenFile() flag, should we do nothing?

How about putting this feature behind a build flag?

As to whether "Go worked fine on Windows for nearly 10 years," it definitely did not, in the event you needed to rename an open file. (It was also broken on Windows 8/10 laptops for the past 7 years.)

I have my own fork of os.OpenFile and syscall.Open already in moby/moby. Why are we being so dismissive here?

On Tue, Oct 8, 2019 at 01:47 Alex Brainman ***@***.***> wrote: I'm really disappointed by this response. I am sorry you feel that way. But did you actually tried to use my package? Alex — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32088?email_source=notifications&email_token=AAGDCZRV72GY4IJQJVJWMYTQNRCIHA5CNFSM4HNPNYIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEATNATA#issuecomment-539414604>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGDCZRNYSMIE6BX77XJVWDQNRCIHANCNFSM4HNPNYIA> .

-- - Brian Goff

I'd suggest this specific proposal be declined. We're not going to introduce a bunch of TOCTOU security bugs for Windows users who relied on the existing behavior of os.OpenFile.

The larger question here is, "how can we expose to Go users the large variety of interesting flags for Windows' CreateFile and NtCreateFile functions?" I expect we'll be able to address those in different proposals, but certainly not by turning them all on by default as this here suggests.

@zx2c4

@zx2c4 did you read the whole thread? We weren't able to identify a single case of a Go user who relies on the existing, undocumented behavior, despite repeated attempts.

It's been a week since #32088 (comment), and there is still very much no clear consensus, which means we should decline this.

Declined.

I've posted a summary of options with pros & cons in #34681 (comment).

always open a stream with FILE_SHARE_DELETE according to the POST gol…

Navigation Menu

Repository navigation

proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows #32088

199 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows #32088

Description

Activity

bradfitz commented on May 16, 2019

alexbrainman commented on May 18, 2019

networkimprov commented on May 18, 2019

alexbrainman commented on May 19, 2019

mattn commented on May 19, 2019

networkimprov commented on May 19, 2019

alexbrainman commented on May 19, 2019

networkimprov commented on May 19, 2019

mattn commented on May 19, 2019

networkimprov commented on May 19, 2019

ericlagergren commented on May 19, 2019

bradfitz commented on May 19, 2019

199 remaining items

cpuguy83 commented on Oct 6, 2019

alexbrainman commented on Oct 8, 2019

networkimprov commented on Oct 8, 2019

cpuguy83 commented on Oct 8, 2019

zx2c4 commented on Oct 9, 2019

networkimprov commented on Oct 9, 2019

rsc commented on Oct 9, 2019

networkimprov commented on Dec 15, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions