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

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

Closed
networkimprov opened this issue May 16, 2019 · 194 comments
Closed

Comments

@networkimprov
Copy link

networkimprov commented May 16, 2019

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

@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 16, 2019
@bradfitz bradfitz added this to the Go1.14 milestone May 16, 2019
@bradfitz
Copy link
Contributor

/cc @alexbrainman

@alexbrainman
Copy link
Member

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

Why should it?

Alex

@networkimprov
Copy link
Author

networkimprov commented May 18, 2019

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(...)

@alexbrainman
Copy link
Member

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

@mattn
Copy link
Member

mattn commented May 19, 2019

@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) }

@networkimprov
Copy link
Author

networkimprov commented May 19, 2019

@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.

@alexbrainman
Copy link
Member

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

@networkimprov
Copy link
Author

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?

@mattn
Copy link
Member

mattn commented May 19, 2019

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.

image

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

@networkimprov
Copy link
Author

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

@ericlagergren
Copy link
Contributor

ericlagergren commented May 19, 2019

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.

@bradfitz
Copy link
Contributor

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.

@guybrand
Copy link

@bradfitz
It would probably not, please refer to my comment here
https://groups.google.com/forum/#!topic/golang-dev/R79TJAzsBfM
or if you prefer I can copy the content here, as there is also a possible solution, although I would not implement it as the default behavior, as this would not be consistent with how windows programs behave but rather a workaround to create a similar cross-os experience.

@networkimprov
Copy link
Author

networkimprov commented May 20, 2019

@guybrand writes in the golang-dev thread:

my program is writing a file called "my.data", and then calls another program and does not wait for it to end ... this program lets say uploads this file which takes 10 seconds (lets call this other program "uploader") .
...
When my program calls .Remove on Windows (FILE_SHARE_DELETE is on):
...
uploader would receive an error telling it the file is removed (probably EOF).

Assuming "uploader" opens the file before "my program" calls os.Remove(), I think you contradicted the Win API docs:

DeleteFile marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

Re "pairing an _open_osfhandle() of the CreateFile to an _fdopen", can you point to example code?

@mattn
Copy link
Member

mattn commented May 20, 2019

If we add FILE_SHARE_DELETE, many programmer will mistakenly use it to simulate Unix behavior. In this case, you can make a function separated by build-constraints for each OSs. And it should return os.NewFile(), use FILE_SHARE_DELETE on Windows.

@networkimprov
Copy link
Author

make a function separated by build-constraints for each OSs... return os.NewFile(), use FILE_SHARE_DELETE on Windows

To retain the functionality of os.OpenFile() this plan appears to require reimplementing all of:

os.OpenFile()
openFileNolog()
openFile()
openDir()
newFile()
fixLongPath()
syscall.Open()
makeInheritSa()

@guybrand
Copy link

guybrand commented May 20, 2019

@networkimprov

@guybrand writes in the golang-dev thread:

my program is writing a file called "my.data", and then calls another program and does not wait for it to end ... this program lets say uploads this file which takes 10 seconds (lets call this other program "uploader") .
...
When my program calls .Remove on Windows (FILE_SHARE_DELETE is on):
...
uploader would receive an error telling it the file is removed (probably EOF).

Assuming "uploader" opens the file before "my program" calls os.Remove(), haven't you contradicted the Win API docs?

DeleteFile marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

Re "pairing an _open_osfhandle() of the CreateFile to an _fdopen", can you point to example code?

I dont see a contradiction with WIndows API, please point what looks like a contridiction.

As for a code sample, I Googled a bit, and found this:
http://blog.httrack.com/blog/2013/10/05/creating-deletable-and-movable-files-on-windows/

BUT
Please note - this will only give you a nix-like behavior internally, any other external program working with it will break it (as it 99% using standard windows API

@guybrand
Copy link

@networkimprov
If you mean "this sounds like "dir my.data" would display the file, as far as I remember this was the behavior until windows .... XP or 7 (dont remember which) and changed since (perhaps the explorer just hides it somehow) - I can retest the next time I launch my windows env. (happens every few week...)

If you mean "this sounds like other processes with a handle to the file would still be able to read and write to the file", I would bet lunch the second you read write to such a file you do get an "EOF" style error - but again need to confirm to be 100% positive.

In either case, my point would be (at this point - I am taking "sides" in the "native like" vs " os-agnostic" point) - even if you implement _fdopen style solution, you would get inconsistency between your service and all other executables you collaborate with, so the use could only be in interaction with other go executables (or rare services that DO use fd's directly).

In other words - your app would be the "smartest kid in class" - that no other kid can understand.
Two examples from many I can think of:
Inputs:
My app downloads a file,the antivirus identifies its harfull and deletes/quarantines (==sort of rename) it, if I use fd - my app would still be able to do whatever it want with it (which is coll, but may end up hitting a virus...)
outputs:
my app pipes a file to another ("uploader" like) service, and deletes it, I even bothered and wrote a tester in go to see that all is working fine - and the test passes.
Now instead of my go test, I use filezilla, we transfter, dropbx API, whatever
it would fail/not behave in the same manner my test works...

Do you still think changing this to the default behavior makes sense ?

@alexbrainman
Copy link
Member

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

I have never considered that question. I do not know.

The way Go files work on Windows is consistent with all other developers tools I have used in my life. It would be surprising to me, if Go files would work as you propose. I also suspect, it would break many existing programs.

Alex

@networkimprov
Copy link
Author

I also suspect, it would break many existing programs.

@alexbrainman I also suggested a switch to enable it, instead of changing the default.

@bradfitz there is syscall.SocketDisableIPv6, that's not really different from a flag to adjust syscall.Open() behavior.

@guybrand
Copy link

Since syscall_windows*.go states
func Open(path string, mode int, perm uint32) (fd Handle, err error) {
....
sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE)

and type_windows*.go has
FILE_SHARE_DELETE = 0x00000004

All is quite ready, the only question is how to implement the flag.
I do not like the global option as well as its not explicit, and while a single developer may remember he has set os.DeleteUponLastHandleClosed = 1, its not a good practice for long term or multiple developer.
Other options would either be setting a specific reserved number for flags, like in:
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.DELETE_WHEN_FREED, 0600)
whereas DELETE_WHEN_FREED can even be 0 for env. other than windows,

Another option would be to use the perm parameter, which is not supported for windows, this may get a little awkward
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 777)
777 is reserved, so we would either need a 1777 or -777 ro support both systems
to make is readable DELETE_WHEN_FREED | 777

last option I can think of is os.OpenDeletableFile(
Which would os.OpenFile on nix's and
turn
sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE)
on windows

all the above solutions are simple to implement, a little more time to test (cross os), just need a voter...

@xenoscopic
Copy link

xenoscopic commented May 20, 2019

One way to support this behavior without changing the defaults or extending the API surface might be to just accept syscall.FILE_SHARE_DELETE in the flag parameter of os.OpenFile on Windows and fold it into the computed sharemode value. Since the syscall.O_* (and hence os.O_*) flags on Windows use made up values anyway, they could be engineered to avoid colliding with any Windows-specific flags that one wanted to include. Fortunately, they already avoid collisions with syscall.FILE_SHARE_DELETE.

In any event, I would strongly oppose changing the default behavior. Making FILE_SHARE_DELETE the default would put you in the same position as POSIX systems in terms of being unable to ensure race-free filesystem traversal. Having this flag be optional is why Windows doesn't need the equivalent of openat, renameat, readlinkat, etc.

@ianlancetaylor
Copy link
Contributor

I haven't really thought about what we should do here, but I want to make one thing clear:

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

We will not be doing this. That would make it impossible for a single program to use different packages that expect different behavior.

@guybrand
Copy link

@havoc-io
Your suggestion would create error prone programs as syscall.FILE_SHARE_DELETE differs from POSIX std behavior.
Please note the authors of syscall_windows*.go were aware of the FILE_SHARE_DELETE flag (its in there) and decided using it would not be the preferred behavior.
Why?

lets take Liam's code, he
defer fd.Close()
deletes/renames
and then read/writes

so actual sort order is
open
mark as deleted
read/write (and probably cross process/cross go routine read write as well)
close

Current windows behavior alerts the developer "this is wrong", developer would probably push the delete to be defer'ed as well
If you Add FILE_SHARE_DELETE, windows would allow you to "mark as delete even though the file open" BUT another process/goroutine running concurrently that would try to access the file between the delete and the close (which is probably the longer task in this code) would fail
Result: instead of a consistent "you can do that" behavior - you would get a "sometimes - especially when there are many concurrent users it fails and I dont know why.
So your not solving the problem, just handling a specific use case on which the developer "only run that once"
My vote is for the consistent behavior of course...

How is that diff from Posix?
Once marked as deleted the file is no longer on the file table, it only exists as an fd, allowing another routine/process to create a file with the very same name.

I think we should stop looking for a "same solution" as there are differences we will not resolve within go (case sensitive filenames ? :) we will let Linux 5.2 do that...)

Altogether it looks like looking for a solution for a temporary file, if it is, windows supports GetTempFileName which can be considered as a standard solution for a file that is "used and then trashed" .

If on the other hand we wish to allow a developer to defer deletes in windows, that's possible, see my suggestions above, but the developer must take responsibility for that, and understand the consciousness - therefore must be a flag with a good name convention.

@networkimprov
Copy link
Author

The primary use for this feature is to rename an open file after creating it:

fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
_, err = fd.Write(...)
err = fd.Sync()  // file now safe to share
err = os.Rename(path, shared_path)
// os.Close() at program exit

I don't know why you wouldn't add a flag os.O_WRNDEL (windows rename/delete; a no-op on other platforms), and document that mode's differences with Unix; that a deleted file remains in its directory, but can't be opened, until os.Close(). Also mention that moving the file to a temp dir before deleting it provides more Unix-like behavior.

@xenoscopic
Copy link

@guybrand I think perhaps you're misunderstanding my proposal. I'm not advocating that FILE_SHARE_DELETE be enabled by default - in fact I'm opposing it for the reasons mentioned in the second paragraph of my comment. My proposal was for a mechanism that allows users to opt-in to FILE_SHARE_DELETE behavior (on a per-file basis) while still using the os package's file infrastructure. This proposal provides a mechanism for doing so without extending the API surface of any package.

I don't know why you wouldn't add a flag os.O_WRNDEL (windows rename/delete; a no-op on other platforms)

@networkimprov That's essentially what I'm proposing, except that there's no point in defining a new flag since a perfectly valid one already exists: syscall.FILE_SHARE_DELETE. The only implementation change would be to watch for this flag in syscall.Open on Windows and add checks to ensure that no future additions to syscall.O_*/os.O_* flags collide with this flag. The documentation for os.OpenFile could then be updated to reflect acceptance of this flag on Windows.

@guybrand
Copy link

@havoc-io sorry for the misunderstanding, this was my takeout from:
" ... to just accept syscall.FILE_SHARE_DELETE ... into the computed sharemode..."

Adding os.O_WRNDEL matches with my second suggestion apart from not being explicit enough for the developer in terms of "what would be the behavior", perhaps os.WADRNDEL - Windows allow deferred rename/delete) .

@rsc
Copy link
Contributor

rsc commented Oct 3, 2019

@rsc could I ask which of the technical arguments made against the proposal were not refuted?

I'm sorry, but that's not how the proposal process works. It is not enough to "refute" other people's arguments. "The goal of the proposal process is to reach general consensus about the outcome in a timely manner." There is no clear consensus about the path forward here. Technical arguments have been made, and they did not convince key Go developers who have made significant contributions to the Windows port (namely, @alexbrainman and @mattn). In addition to the lack of clear consensus, there is no clear sign of urgency to do something today: Go worked fine on Windows for nearly 10 years before this issue was filed. As I understand it, leaving everything alone means Go will continue to work as well as it always has.

I filed #34681 to provide an easier way to open files with FILE_SHARE_DELETE in the (still likely) event that this one is declined. It seemed more helpful to start a new thread limited to that idea than to continue this one, which has gotten very long.

@networkimprov
Copy link
Author

@rsc, before you decline this, let's hear what Alex thinks of your O_ALLOW_DELETE proposal.

Early in this discussion, he was against a new os.OpenFile() flag that sets file_share_delete, for the same reasons he disagreed with your os.Create/Open/OpenFile() suggestion. He is concerned that other programs assume that no one ever opens files that way, because MSVC fopen() cannot do so. Those (still unspecified) programs will therefore break Go programs which set the flag.

@alexbrainman
Copy link
Member

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 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, 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

@cpuguy83
Copy link

cpuguy83 commented Oct 6, 2019 via email

@alexbrainman
Copy link
Member

I'm really disappointed by this response.

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

Alex

@networkimprov
Copy link
Author

@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.)

@cpuguy83
Copy link

cpuguy83 commented Oct 8, 2019 via email

@zx2c4
Copy link
Contributor

zx2c4 commented Oct 9, 2019

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.

@networkimprov
Copy link
Author

@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.

@andybons andybons modified the milestones: Go1.14, Proposal Oct 9, 2019
@rsc
Copy link
Contributor

rsc commented Oct 9, 2019

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

Declined.

@networkimprov
Copy link
Author

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

@golang golang locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests