Use O_CLOFORK where O_CLOEXEC is used. #1988
Conversation
We only fork to exec gzip so it's safe. Signed-off-by: Ricardo Branco <[email protected]>
Signed-off-by: Ricardo Branco <[email protected]>
The O_CLOFORK flag can be used for the log fd which is not needed on the child process. Signed-off-by: Ricardo Branco <[email protected]>
We don't fork nor exec so it's safe. Signed-off-by: Ricardo Branco <[email protected]>
We only fork to exec so it's safe. Signed-off-by: Ricardo Branco <[email protected]>
We only fork to exec so it's safe. Signed-off-by: Ricardo Branco <[email protected]>
|
Thank you for taking the time to contribute to FreeBSD! Some of files have special handling: Important @concussious wants to review changes to share/man/ |
|
I do not think it is useful to add O_CLOFORK in random places just because we can. |
O_CLOEXEC is used on places where we don't want leaking file descriptors on exec. There's no harm in making sure we don't leak them on fork. We could even s/O_CLOEXEC/O_CLOFORK/ with the fixed semantics where O_CLOFORK implies O_CLOEXEC: https://austingroupbugs.net/view.php?id=1851 |
We might, but again, what is the purpose? It is good and correct as is now. What problem(s) would the change fix? What improvements can be from there? Also, this makes the code unportable to platforms that do not have O_CLOFORK. Minor and non-blocking, but not ignorable for a change that IMO makes no difference otherwise. |
Just preventing file descriptor leaks. We use O_CLOEXEC on sort(1) even though we don't fork nor exec.
We can do what NetBSD does by defining the flag to 0 if not defined. I added a new commit replacing O_CLOEXEC with O_CLOFORK for rtld(8). This change is non-trivial but at least all tests passed and nothing seems broken on my test VM. |
|
So this is all pointless. I object for pushing this into FreeBSD, as a note for whoever process the list of PRs. |
This flag unofficially implies O_CLOEXEC on *BSD, Illumos & Solaris. Signed-off-by: Ricardo Branco <[email protected]>
2 participants
I identified these low-hanging fruit patterns where
O_CLOFORKcan be safely OR-ed withO_CLOEXEC:openalmost immediately followed byclosein the same function.This is the case for the following binaries:
Also update filemon(4) to establish that we can use O_CLOFORK on the logging file descriptor since it's not needed by the child process.
As a consequence of the above, also fix script(1)
Not trivial but tests passed:
This PR is limited only to O_CLOEXEC uses found by a simple
find *bin -name \*.c -exec grep CLOEXEC {} +. After this is merged, another PR will follow for lib.I'll be doing the same for NetBSD, OpenBSD & DragonflyBSD and will link the discussions here.
Ref: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=286843
TODO
#define O_CLOFORK 0if not defined to support other platforms.