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: race between File.Close, File.Write, and other Opens #7970

Closed
rsc opened this issue May 11, 2014 · 15 comments
Closed

os: race between File.Close, File.Write, and other Opens #7970

rsc opened this issue May 11, 2014 · 15 comments
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented May 11, 2014

If you have a goroutine calling f.Close at the same time another calls f.Write, it can
happen that the f.Close happens, then an unrelated os.Open reuses the fd, then the
f.Write writes to the wrong fd. The timing would have to be just right, since Close does
set f.fd = -1, but it could happen.  We could decide this is not worth protecting
against, or we could introduce an rwlock on the File that must be rlocked across all
fd-using method implementations and wlocked by Close.

Investigate for 1.4.

Related to issue #5792.
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 1, 2014

Comment 1:

Labels changed: added repo-main.

@rsc
Copy link
Contributor Author

@rsc rsc commented Oct 6, 2014

Comment 2:

Too late to make such a fundamental change.

Labels changed: added release-go1.5, removed release-go1.4.

@rsc rsc added accepted labels Oct 6, 2014
@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@bradfitz bradfitz removed the release-go1.5 label Dec 16, 2014
@rsc rsc removed accepted labels Apr 14, 2015
@larzconwell
Copy link
Contributor

@larzconwell larzconwell commented May 17, 2015

If we do decide to add a rw mutex I've added the following CL that implements that in os.file https://go-review.googlesource.com/#/c/10186/

@gopherbot
Copy link

@gopherbot gopherbot commented May 17, 2015

CL https://golang.org/cl/10186 mentions this issue.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 22, 2015

Using an RWMutex for file.fd will mean that if a Read hangs in the kernel waiting for data, a Close on the same file will hang waiting for the read to complete. I don't think that is desirable.

I think we should postpone this until issue #6817 is fixed.

@rsc rsc modified the milestones: Go1.6Early, Go1.5 Jun 26, 2015
@rsc
Copy link
Contributor Author

@rsc rsc commented Jun 26, 2015

#6817 might help with this, but I'm not convinced it is always available in all cases. Maybe I'm wrong.

We might be able to just keep living with this. Certainly we don't hear about it much.

If we need to fix things, and AIO is not good enough (and I suspect it's not), one possibility would be to create an unusable fd somehow (for example call socket without binding it anywhere, or maybe call pipe and close the read end and use the write fd) and instead of using Close(fd) use Dup(badfd, fd). That would close the original fd and make f.fd unusable, without leaving the fd available for reuse immediately. The Dup'ed copy of badfd could be properly closed when we know all the i/o has stopped. The problem with this, I suspect, is that Close of a file open for writing can return information about write errors that are as yet unreported, and I suspect Dup onto an existing fd does not return those errors.

Like I said, this is very difficult to fix. I expect that most other languages just live with this the same as Go. I don't know that there's any fix that's not worse than the problem.

@rsc
Copy link
Contributor Author

@rsc rsc commented Jun 26, 2015

It would be nice to know what other languages do, if anyone knows.

@tv42
Copy link

@tv42 tv42 commented Jun 26, 2015

I think it's worth noting that nothing in os.File documentation says its methods are safe to call from multiple goroutines. net.Conn has that guarantee, os.File does not.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 28, 2015

I'm not really sure which languages permit concurrent read/close access in any way. I think for most languages it is undefined behavior. For kernel file descriptors it is permitted, but of course the same race is possible.

In the net package we handle the same problem with the code in net/fd_mutex.go. As far as I can see the same approach would work in the os package. This would mean extending the problem of errClosing (#4373) to the os package.

The fix for issue #6817 probably requires some set of helper threads for I/O. AIO is not really possible on GNU/Linux systems. There is no separate supported kernel interface for it; the aio_read functions and friends are implemented by simply using a thread pool for I/O.

@rsc
Copy link
Contributor Author

@rsc rsc commented Nov 24, 2015

Continuing my Dup comment from above, @aclements points out taht we can avoid losing the error from syscall.Close by doing:

newfd := syscall.Dup(fd)
syscall.Dup2(badfd, fd)
err := syscall.Close(newfd)
... close fd (now equivalent to badfd) once ref count drops to 0 ...

Of course, this would mean that if you run out of file descriptors, it becomes impossible to close a file. We could do this only for fd's with pending operations, of course, but then there's a little-exercised code path lurking.

This seems extremely low priority. Going to move to Unplanned until that changes.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 25, 2017

CL https://golang.org/cl/41674 mentions this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2017

Change https://golang.org/cl/65490 mentions this issue: os/exec: remove protection against simultaneous Wait/Write

gopherbot pushed a commit that referenced this issue Sep 22, 2017
CL 31148 added code to protect again simultaneous calls to Close and
Wait when using the standard input pipe, to fix the race condition
described in issue #9307. That issue is a special case of the race
between Close and Write described by issue #7970. Since issue #7970
was not fixed, CL 31148 fixed the problem specific to os/exec.

Since then, issue #7970 has been fixed, so the specific fix in os/exec
is no longer necessary. Remove it, effectively reverting CL 31148 and
followup CL 33298.

Updates #7970
Updates #9307
Updates #17647

Change-Id: Ic0b62569cb0aba44b32153cf5f9632bd1f1b411a
Reviewed-on: https://go-review.googlesource.com/65490
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Miguel Bernabeu <miguelbernadi@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Joe Tsai <joetsai@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 26, 2017

Change https://golang.org/cl/66150 mentions this issue: internal/poll: don't return from Close until descriptor is closed

gopherbot pushed a commit that referenced this issue Sep 26, 2017
This permits the program to reliably know that when the Close method
returns, the descriptor has definitely been closed. This matters at
least for listeners.

Fixes #21856
Updates #7970

Change-Id: I1fd0cfd2333649e6e67c6ae956e19fdff3a35a83
Reviewed-on: https://go-review.googlesource.com/66150
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <joetsai@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 13, 2017

Change https://golang.org/cl/83715 mentions this issue: net, os: don't wait for Close in blocking mode

gopherbot pushed a commit that referenced this issue Dec 14, 2017
Updates #7970
Updates #21856
Updates #23111

Change-Id: I0cd0151fcca740c40c3c976f941b04e98e67b0bf
Reviewed-on: https://go-review.googlesource.com/83715
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 14, 2017

Change https://golang.org/cl/83995 mentions this issue: os: don't wait for Close if the File was returned by NewFile

gopherbot pushed a commit that referenced this issue Dec 14, 2017
os.NewFile doesn't put the fd into non-blocking mode.
In most cases, an *os.File returned by os.NewFile is in blocking mode.

Updates #7970
Updates #21856
Updates #23111

Change-Id: Iab08432e41f7ac1b5e25aaa8855d478adb7f98ed
Reviewed-on: https://go-review.googlesource.com/83995
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Dec 14, 2018
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
7 participants
You can’t perform that action at this time.