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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3652 bytes --]

On Wed, May 20, 2026 at 01:36:48PM +0200, Stefano Brivio wrote:
> On Wed, 20 May 2026 11:04:58 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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.

> $ 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.

> But as I was mentioning in that path we don't use CLONE_FILES anyway,
> so that's not relevant.
> 
> -- 
> Stefano
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]