Re: [PATCH 1/3] treewide: Add SOCK_CLOEXEC to accept() calls that are missing it

On Wed, May 20, 2026 at 04:22:10PM +0200, Stefano Brivio wrote:

On Wed, 20 May 2026 22:52:47 +1000 David Gibson wrote:

...

On Wed, May 20, 2026 at 01:36:48PM +0200, Stefano Brivio wrote:

...

On Wed, 20 May 2026 11:04:58 +1000 David Gibson wrote:

...

On Wed, May 20, 2026 at 02:37:02AM +0200, Stefano Brivio wrote:

...

On Mon, 18 May 2026 12:28:57 +1000 David Gibson wrote:

...

On Sat, May 16, 2026 at 05:46:11PM +0200, Stefano Brivio wrote: > On Wed, 13 May 2026 14:14:21 +1000 > David Gibson wrote: > > > Generally we try to set the O_CLOEXEC flag on every fd we create. This > > seems to be generally accepted security best practice these days, and we > > never fork(), so certainly have no need to pass fds to children. > > But we do clone() with CLONE_FILES (even though when we clone() to call > execvp() later, we don't set CLONE_FILES), so, even though I don't see > a reason to skip O_CLOEXEC for c->fd_tap, this conclusion shouldn't be > automatic from the fact we don't fork().

So, I did think about that when wrote it, but went for the short version rather than saying clone() with CLONE_FILES doesn't count.

Now, I realised that we've both fallen for the trap again, forgetting that this has nothing to do with fork() or clone() and is, as it says right there in the name, about exec().

No, wait, I didn't fall for it, not this time. :) That's why I was mentioning that when we call clone() and execvp() later (which would be

Uh...? I'm pretty sure the only execve(2) in the entire program is where we spawn passt.avx2. That's essentially the very first thing we do, long before this point.

Well, grep would beg to differ, as we don't call execve() at all, but:

I meant the system call execve(2), which execv() and execvp() are library wrappers around.

Oops, I missed the (2).

...

...

$ grep execv *.c | grep -v qrap arch.c: execv(new_path, argv); pasta.c: execvp(a->exe, a->argv);

Ah, I did miss the one in pasta_spawn_cmd(). Of course, we definitely don't want to leak our internal fds into the spawned command, so CLOEXEC is what we want.

...

O_CLOEXEC (or lack thereof) also matters on execvp().

...

...

the only path that matters), we don't set CLONE_FILES anyway.

CLONE_FILES is irrelevant, it's lost during execve(2).

Yes, but if you first clone(), which we actually do before calling pasta_spawn_cmd(), and then execvp(), CLONE_FILES on clone() *would* matter, because the cloned process would inherit the open files, and the process started by execvp() would then get those files as well.

No, it doesn't matter. If you clone() without CLONE_FILES, the new thread/process gets a copy of the handles, which do or don't survive exec() depending on O_CLOEXEC. If you clone with CLONE_FILES, the new process shares the fd table. The fd table is unshared again as part of the exec().

From execve(2):

...

• The file descriptor table is unshared, undoing the effect of the CLONE_FILES flag of clone(2).

.. then the now copied files do or don't survive depending on O_CLOEXEC. Either way, O_CLOEXEC has the same final effect.

Ah, right, I forgot about this part. But anyway, O_CLOEXEC is always relevant, as long as we call execvp() not right after start, regardless of having called clone() before.

And in this case (pasta_spawn_cmd()) we call it rather "late", so the fact we don't call fork() is not really relevant for this purpose.

Right. I already removed the reference to fork() in the next vesion. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson