Use O_CLOFORK where O_CLOEXEC is used.

[ricardobranco777]

I identified these low-hanging fruit patterns where O_CLOFORK can be safely OR-ed with O_CLOEXEC:

• There are no fork calls at all.
• There's only fork+exec.
• There's an open almost immediately followed by close in the same function.

This is the case for the following binaries:

• edquota(8): We only fork to exec so it's safe.
• pax(1): We only fork to exec gzip so it's safe.
• pw(8): We only fork to exec so it's safe.
• sort(1): No fork calls.

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:

• rtld(8)

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

• Add #define O_CLOFORK 0 if not defined to support other platforms.
• Replace O_CLOEXEC with O_CLOFORK (which now implies O_CLOEXEC according to https://austingroupbugs.net/view.php?id=1851) ?

[github-actions]

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/

[kostikbel]

I do not think it is useful to add O_CLOFORK in random places just because we can.
O_CLOFORK was created to solve very specific problem (if not the problem of POSIX compliance, but I regret).
In all places that you patched, there is no such problem to solve, and your comment states this straight.
So I suggest to not do this, it serves no purpose.

[ricardobranco777]

I do not think it is useful to add O_CLOFORK in random places just because we can. O_CLOFORK was created to solve very specific problem (if not the problem of POSIX compliance, but I regret). In all places that you patched, there is no such problem to solve, and your comment states this straight. So I suggest to not do this, it serves no purpose.

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

[kostikbel]

I do not think it is useful to add O_CLOFORK in random places just because we can. O_CLOFORK was created to solve very specific problem (if not the problem of POSIX compliance, but I regret). In all places that you patched, there is no such problem to solve, and your comment states this straight. So I suggest to not do this, it serves no purpose.

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. These unofficial semantics are valid on *BSD, Illumos & Solaris.

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.

[ricardobranco777]

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?

Just preventing file descriptor leaks. We use O_CLOEXEC on sort(1) even though we don't fork nor exec.

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.

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.

[kostikbel]

So this is all pointless. I object for pushing this into FreeBSD, as a note for whoever process the list of PRs.
There is no descriptors leaks.