-
Notifications
You must be signed in to change notification settings - Fork 3k
Add support for POSIX O_CLOFORK #1698
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
base: main
Are you sure you want to change the base?
Conversation
Skimming the thread, the main objection seems to be that the feature adds extra memory accesses to certain hot paths. I think this is not so true in our case, we already pay some of these overheads. We have 7 bits available for per-fd flags (struct filedescent is pretty fat anyway since it stores capsicum rights), and we already have to iterate over fds upon fork since kqueue descriptors implicitly have O_CLOFORK set. XNU seems to implement this flag as well. I don't have strong feelings on whether it's very useful or not, but at a glance the implementation doesn't look too complex. |
I do share the sentiment that the feature is silly. But the implementation should be straighforward, since the main pain point is fdcopy() where we already do the pass over all fds in the parent. IMO it should be finished if only to have a checkpoint for POSIX compliance. |
6102aa1
to
2fb5caa
Compare
CDDL-licensed code from illumos should be placed under cddl/contrib/opensolaris. It's ok for the build outputs to be installed under /usr/tests, but the code should be excluded from the build if |
2854c77
to
77c1f74
Compare
The build is not done automatically. To test:
|
a3d476d
to
7aeab4f
Compare
The
|
7aeab4f
to
a7344b7
Compare
oclo tests results all PASSED:
|
Why not build it automatically? If it's not connected to the build, we won't catch regressions easily. |
a7344b7
to
f909915
Compare
It now builds but doesn't run automatically because |
It's ok to modify the test code to add or exclude checks, typically with |
f909915
to
1d489e3
Compare
I just used I see the |
9663d87
to
269289f
Compare
Taken from last commit bb9475a199514dcace79d04d02c1eff05d65b94f from https://github.com/illumos/illumos-gate/tree/master/usr/src/test/os-tests/tests/oclo
269289f
to
08d3a46
Compare
08d3a46
to
f1a6f16
Compare
I brought the PR to the OpenBSD folks. Thread at https://marc.info/?l=openbsd-tech&m=175054703914823&w=2 Issue open at Austin Group: |
aa21289
to
a9507fa
Compare
80ddb32
to
7876748
Compare
The last commit works on both FreeBSD & DragonflyBSD as the oclo tests pass, but on FreeBSD init is now inmortal and refuses to die: EDIT: Fixed in the force-push. |
b7f4f1f
to
c5c14ec
Compare
It's working fine for me. All relevant tests touched in this PR pass. The last commits need review though. Will extend the tests for OpenBSD as they seem also interested in adding the flag and I think we should default to their behaviour because of the security issue mentioned in the POSIX issue. |
Asked Oxide Computers via e-mail to relicense the oclo tests to some BSD-like license so they can be incorporated into OpenBSD following objections over CDDL license. Also in omniosorg/illumos-omnios#1589 |
lib/libc/gen/fdopendir.c
Outdated
return (NULL); | ||
|
||
if ((flags & FD_CLOEXEC) == 0 && | ||
_fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like this, it is racy. Suppose that another thread manipulates the same fd flag in parallel. Then our flags is stale and we might still clear a flag set by the other thread.
I suspect we need a new fcntl op to just set the passed bits of flags, like F_ORFD or such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like this, it is racy. Suppose that another thread manipulates the same fd flag in parallel. Then our flags is stale and we might still clear a flag set by the other thread.
I suspect we need a new fcntl op to just set the passed bits of flags, like F_ORFD or such.
Can it be done in another PR? The scope is already wide for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, and I think that existing merged PR should not be continued.
But this patch for libc should not go as is, F_ORFD (and might be F_XORFD to clear) needs to be used there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped the commit.
@@ -2748,6 +2748,12 @@ fdcloseexec(struct thread *td) | |||
fdfree(fdp, i); | |||
(void) closefp(fdp, i, fp, td, false, false); | |||
FILEDESC_UNLOCK_ASSERT(fdp); | |||
} else if (fde->fde_flags & UF_FOCLOSE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not clear the flag unconditionally? To not dirty the cache lines for *fde?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not clear the flag unconditionally? To not dirty the cache lines for *fde?
Yes. It's also cheaper this way if we eventually want to use a lock here.
Additional discussion in: https://austingroupbugs.net/view.php?id=1851 Also: https://marc.info/?l=openbsd-tech&m=175054923615444&w=2
c5c14ec
to
1ec1bd5
Compare
This PR:
This one looks perhaps deceptively simple to implement but it's passing all tests (hundreds of them) from a suite adapted from Illumos.
This may be useful for Golang, Rust, Swift (and other languages):
Command::spawn
on a newly-written file can fail with ETXTBSY due to racing with itself on Unix rust-lang/rust#114554This interface was called stupid by opinionated Linux folks and perhaps they're right, so I want an informed opinion before going any further. I had my fun anyway. Here are the relevant links with discussion:
DONE
dup3
F_SETFD
found withgrep -Er 'F_SETFD, (1|FD_CLOEXEC|fcntl)' *
NOT NEEDED
"f"
infopen
. It's not POSIX and not even Solaris/Illumos implement it.NOTES
POSIX References:
Illumos references:
Fixes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=286843
To test: