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: closing os.Stderr causes panic to write to arbitrary files referenced by fd 2 #15970

Closed
as opened this issue Jun 6, 2016 · 12 comments
Closed

Comments

@as
Copy link
Contributor

@as as commented Jun 6, 2016

  1. Version: Reproduced on two setups below
    (a) 1.7beta1 plan9/amd64
    (b) 1.6.2 linux/arm
  2. What did you do?
    I first closed standard error, then created a new file, and lastly, called panic(). The panic was written to the created file.
  3. Repeat steps:
    The snippet doesn't produce verifiable results on the Go playground out of the box, compile locally to reproduce. The program creates the file "/tmp/test2", if the issue is reproduced then this file will contain panic output from the runtime.

https://play.golang.org/p/D2mIGkI4Oo

  1. What did you expect to see?
    I expected no panic output to be written to the newly-created file.
  2. What did you see instead?
    The panic output was written to a newly-created file.

Notes:

In 1.7beta1 (and possibly below) this file contains a function that references fd 2 numerically:
$GOROOT/src/runtime/write_err.go

The godoc for os does not mention file descriptor reuse

Not tested on Windows or other operating systems/architectures.

Panics are not written if the file is opened with os.Open() or in an explicit read-only mode.

@davecheney

This comment has been minimized.

Copy link
Contributor

@davecheney davecheney commented Jun 6, 2016

I don't think this can be fixed without breaking the Go 1 contract. If we had our time over again, os.Stderr would not be an os.File.

@as

This comment has been minimized.

Copy link
Contributor Author

@as as commented Jun 6, 2016

It seems like an esoteric case, since I haven't found a prior issue logged for this.

However, the behavior could be documented to provide clarity since os package has a platform-independent tone. I don't have access to a Windows right now, but "handles" may have an arbitrary numbering that make Windows unaffected. If so, os.Stderr.Close() and panic() lose their consistency between Windows and UNIX.

Another question I don't know the answer to is whether this potentiates "stderr closers" to some information disclosure on the condition that a remote party can influence what becomes fd 2 and is able to trigger panics or other low-level printing routines to transmit data back to them through the fd. It seems far-fetched but worth noting.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 6, 2016

I agree with @davecheney; I don't think there is anything we can do here besides document it.

@ianlancetaylor ianlancetaylor added this to the Go1.7Maybe milestone Jun 6, 2016
@as

This comment has been minimized.

Copy link
Contributor Author

@as as commented Jun 6, 2016

I can ascertain the behavior for a few Windows client and server systems using go1.7beta1 within the next few days.

@extemporalgenome

This comment has been minimized.

Copy link
Contributor

@extemporalgenome extemporalgenome commented Jun 6, 2016

We could open/dup a file-descriptor in place of the stdio fd's after they're closed, after they're closed, so that the fd's are not reused for any semantic purpose.

@adg

This comment has been minimized.

Copy link
Contributor

@adg adg commented Jun 6, 2016

I don't think there's a lot of value in guarding against this. What if someone legitimately wants to close os.Stderr and use fd 2 for something else? It's such a corner case that anything we do will be wrong for someone.

I vote in favor of documenting this on os.Stdout/Stderr. (is os.Stdin also affected?)

@as

This comment has been minimized.

Copy link
Contributor Author

@as as commented Jun 7, 2016

(is os.Stdin also affected?)

https://play.golang.org/p/jHoq_1UuSB

It depends on the exact definition of affected. The fd 0 is reused, but it also seems improbable for unsuspecting users to trigger the behavior, especially compared to fd 2, which has panic writing to it.

Is there a runtime component which uses a hard-coded fd 0 in the same manner that panic uses fd 2? (I couldn't find such usage in the source)

@as

This comment has been minimized.

Copy link
Contributor Author

@as as commented Jun 8, 2016

Tested on a few Windows systems, FD reuse varies across releases. W7 and it's server counterpart wouldn't miswrite to the file, the rest did.

https://play.golang.org/p/_DmUtoaBii

VERSION    STRING             RESULT
6.1.7601   "Windows 7"        Not Reproduced
6.1.7601   "Windows 2008 R2"  Not Reproduced
6.3.9600   "Windows 8.1"      Reproduced
6.3.9600   "Windows 2012 R2"  Reproduced
10.0.10586 "Windows 10"       Reproduced

Possible documented workaround could be opening os.DevNull immediately after closing os.Stderr. This makes the issue non-reproducible on all tested systems so far.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 9, 2016

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

@as

This comment has been minimized.

Copy link
Contributor Author

@as as commented Jun 9, 2016

// Note that the Go runtime writes to standard error for panics and crashes;
// closing Stderr may cause those messages to go elsewhere, perhaps
// to a file opened later.

On linux/arm go1.6 and plan9/amd64 go1.7beta1 the file opened later can be the fd associated with a Conn resulting from a net.Dial. Would it be sufficient to use "file" given that a Conn can do this as well? Perhaps "underlying file descriptor 2" would clarify that this isn't exclusively an os.File.

// closing Stderr causes those messages to go to the underlying file descriptor 2, which may
// reference a new file or connection opened later.

https://play.golang.org/p/fyMX_Htckf

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Jun 9, 2016

Yap, I agree too -- I made similar comments in Gerrit. @as would it possible for you to post the comments on the Gerrit code review instead? That way @ianlancetaylor can address them there instead of the CL being submitted without the suggested changes, and him later on discovering your comments later on Github, if he might not have gotten the notification.

@as

This comment has been minimized.

Copy link
Contributor Author

@as as commented Jun 9, 2016

@odeke-em
Done.

@gopherbot gopherbot closed this in a8c6c48 Jun 9, 2016
@golang golang locked and limited conversation to collaborators Jun 9, 2017
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.