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

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().  I'll update the message.

> I spent some time on it and I really couldn't find a reason why we
> don't have O_CLOEXEC there, so probably there isn't any, and I think
> this patch is fine.
> 
> I would just change this paragraph to "[...] these days, and we don't
> need to pass file descriptors to children."
> 
> > A handful of accept4() calls on Unix sockets are missing the SOCK_CLOEXEC
> > flag to set this though.  Add the missing flag.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  repair.c | 5 +++--
> >  tap.c    | 4 ++--
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/repair.c b/repair.c
> > index 69c53077..3e0e3e0a 100644
> > --- a/repair.c
> > +++ b/repair.c
> > @@ -87,7 +87,7 @@ int repair_listen_handler(struct ctx *c, uint32_t events)
> >  	/* Another client is already connected: accept and close right away. */
> >  	if (c->fd_repair != -1) {
> >  		int discard = accept4(c->fd_repair_listen, NULL, NULL,
> > -				      SOCK_NONBLOCK);
> > +				      SOCK_NONBLOCK | SOCK_CLOEXEC);
> >  
> >  		if (discard == -1)
> >  			return errno;
> > @@ -99,7 +99,8 @@ int repair_listen_handler(struct ctx *c, uint32_t events)
> >  		return EEXIST;
> >  	}
> >  
> > -	if ((c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL, 0)) < 0) {
> > +	if ((c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL,
> > +				    SOCK_CLOEXEC)) < 0) {
> >  		rc = errno;
> >  		debug_perror("accept4() on TCP_REPAIR helper listening socket");
> >  		return rc;
> > diff --git a/tap.c b/tap.c
> > index 0920a325..e7cac9df 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1477,7 +1477,7 @@ void tap_listen_handler(struct ctx *c, uint32_t events)
> >  	/* Another client is already connected: accept and close right away. */
> >  	if (c->fd_tap != -1) {
> >  		int discard = accept4(c->fd_tap_listen, NULL, NULL,
> > -				      SOCK_NONBLOCK);
> > +				      SOCK_NONBLOCK | SOCK_CLOEXEC);
> >  
> >  		if (discard == -1)
> >  			return;
> > @@ -1490,7 +1490,7 @@ void tap_listen_handler(struct ctx *c, uint32_t events)
> >  		return;
> >  	}
> >  
> > -	c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, 0);
> > +	c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, SOCK_CLOEXEC);
> >  
> >  	if (!getsockopt(c->fd_tap, SOL_SOCKET, SO_PEERCRED, &ucred, &len))
> >  		info("accepted connection from PID %i", ucred.pid);
> 
> -- 
> 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 --]