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: 4434 bytes --]

On Wed, May 20, 2026 at 04:22:10PM +0200, Stefano Brivio wrote:
> On Wed, 20 May 2026 22:52:47 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <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.
> 
> 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

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