syscall/plan9: remove spooky fd action at a distance #43533

oridb · 2021-01-06T02:32:03Z

Change Plan 9 fork/exec to use the O_CLOEXEC file
descriptor, instead of relying on spooky at a
distance.

Historically, Plan 9 has set the O_CLOEXEC flag on
the underlying channels in the kernel, rather
than the file descriptors -- if two fds pointed
at a single channel, as with dup, changing the
flags on one of them would be observable on the
other.

The per-Chan semantics are ok, if unexpected,
when a chan is only handled within a single
process, but this isn't always the case.

Forked processes share Chans, but even more of
a problem is the interaction between /srv and
OCEXEC, which can lead to unexectedly closed
file descriptors in completely unrelated
proceses. For example:

func exists() bool {
	// If some other thread execs here,
	// we don't want to leak the fd, so
	// open it O_CLOEXEC
	fd := Open("/srv/foo", O_CLOEXEC)
	if fd != -1 {
		Close(fd)
		return true
	}
	return false
}

would close the connection to any file descriptor
(maybe even for the root fs) in ALL other processes
that have it open if an exec were to happen(!),
which is quite undesriable.

As a result, 9front will be changing this behavior
for the next release.

Go is the only code observed so far that relies on
this behavior on purpose, and It's easy to make the
code work with both semantics: simply using the file
descriptor that was opened with O_CEXEC instead of
throwing it away.

So we do that here.

Fixes #43524

Change Plan 9 fork/exec to use the O_CLOEXEC file descriptor, instead of relying on spooky at a distance. Historically, Plan 9 has set the O_CLOEXEC flag on the underlying channels in the kernel, rather than the file descriptors -- if two fds pointed at a single channel, as with dup, changing the flags on one of them would be observable on the other. The per-Chan semantics are ok, if unexpected, when a chan is only handled within a single process, but this isn't always the case. Forked processes share Chans, but even more of a problem is the interaction between /srv and OCEXEC, which can lead to unexectedly closed file descriptors in completely unrelated proceses. For example: func exists() bool { // If some other thread execs here, // we don't want to leak the fd, so // open it O_CLOEXEC fd := Open("/srv/foo", O_CLOEXEC) if fd != -1 { Close(fd) return true } return false } would close the connection to any file descriptor (maybe even for the root fs) in ALL other processes that have it open if an exec were to happen(!), which is quite undesriable. As a result, 9front will be changing this behavior for the next release. Go is the only code observed so far that relies on this behavior on purpose, and It's easy to make the code work with both semantics: simply using the file descriptor that was opened with O_CEXEC instead of throwing it away. So we do that here.

google-cla · 2021-01-06T02:32:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

oridb · 2021-01-06T02:36:39Z

@googlebot I signed it!

gopherbot · 2021-01-06T02:40:10Z

This PR (HEAD: 96bb21b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/281833 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2021-01-06T02:42:21Z

Message from Go Bot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://golang.org/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-01-06T10:47:02Z

Message from David du Colombier:

Patch Set 1: Run-TryBot+1 Code-Review+2

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-01-06T10:47:03Z

Message from Go Bot:

Patch Set 1:

SlowBots beginning. Status page: https://farmer.golang.org/try?commit=7020cd33

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-01-06T14:50:21Z

Message from Go Bot:

Patch Set 1: TryBot-Result-1

1 of 22 SlowBots failed:
Failed on plan9-arm: https://storage.googleapis.com/go-build-log/7020cd33/plan9-arm_0adc1772.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

SlowBot builds that ran:

plan9-arm

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-01-14T16:10:10Z

Message from Ori Bernstein:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-01-14T18:54:12Z

Message from Ian Lance Taylor:

Patch Set 1: Trust+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-01-24T09:46:06Z

Message from Richard Miller:

Patch Set 1: Code-Review+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-02-08T15:41:44Z

Message from Jacob Moody:

Patch Set 1: Code-Review+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/281833.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-02-08T15:59:11Z

This PR is being closed because golang.org/cl/281833 has been merged.

Change Plan 9 fork/exec to use the O_CLOEXEC file descriptor, instead of relying on spooky at a distance. Historically, Plan 9 has set the O_CLOEXEC flag on the underlying channels in the kernel, rather than the file descriptors -- if two fds pointed at a single channel, as with dup, changing the flags on one of them would be observable on the other. The per-Chan semantics are ok, if unexpected, when a chan is only handled within a single process, but this isn't always the case. Forked processes share Chans, but even more of a problem is the interaction between /srv and OCEXEC, which can lead to unexectedly closed file descriptors in completely unrelated proceses. For example: func exists() bool { // If some other thread execs here, // we don't want to leak the fd, so // open it O_CLOEXEC fd := Open("/srv/foo", O_CLOEXEC) if fd != -1 { Close(fd) return true } return false } would close the connection to any file descriptor (maybe even for the root fs) in ALL other processes that have it open if an exec were to happen(!), which is quite undesriable. As a result, 9front will be changing this behavior for the next release. Go is the only code observed so far that relies on this behavior on purpose, and It's easy to make the code work with both semantics: simply using the file descriptor that was opened with O_CEXEC instead of throwing it away. So we do that here. Fixes golang#43524 Change-Id: I4887f5c934a5e63e5e6c1bb59878a325abc928d3 GitHub-Last-Rev: 96bb21b GitHub-Pull-Request: golang#43533 Reviewed-on: https://go-review.googlesource.com/c/go/+/281833 Reviewed-by: David du Colombier <[email protected]> Reviewed-by: Richard Miller <[email protected]> Reviewed-by: Jacob Moody <[email protected]> Run-TryBot: David du Colombier <[email protected]> Trust: Ian Lance Taylor <[email protected]>

google-cla bot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label Jan 6, 2021

google-cla bot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Jan 6, 2021

gopherbot closed this Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

syscall/plan9: remove spooky fd action at a distance #43533

syscall/plan9: remove spooky fd action at a distance #43533

Uh oh!

oridb commented Jan 6, 2021

Uh oh!

google-cla bot commented Jan 6, 2021

Uh oh!

oridb commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 14, 2021

Uh oh!

gopherbot commented Jan 14, 2021

Uh oh!

gopherbot commented Jan 24, 2021

Uh oh!

gopherbot commented Feb 8, 2021

Uh oh!

gopherbot commented Feb 8, 2021

Uh oh!

Uh oh!

syscall/plan9: remove spooky fd action at a distance #43533

syscall/plan9: remove spooky fd action at a distance #43533

Uh oh!

Conversation

oridb commented Jan 6, 2021

Uh oh!

google-cla bot commented Jan 6, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

oridb commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 6, 2021

Uh oh!

gopherbot commented Jan 14, 2021

Uh oh!

gopherbot commented Jan 14, 2021

Uh oh!

gopherbot commented Jan 24, 2021

Uh oh!

gopherbot commented Feb 8, 2021

Uh oh!

gopherbot commented Feb 8, 2021

Uh oh!

Uh oh!