Re: [devel] [PATCH hasher-priv v3 3/7] chrootuid: explicitly reset signal mask before forking off payload

["Dmitry V. Levin" <ldv@altlinux.org>]

On Fri, Dec 03, 2021 at 06:03:31PM +0300, Arseny Maslennikov wrote:
> On Wed, Dec 01, 2021 at 10:23:37PM +0300, Dmitry V. Levin wrote:
> > On Tue, Aug 24, 2021 at 11:24:32AM +0300, Arseny Maslennikov wrote:
> > > Signed-off-by: Arseny Maslennikov <arseny@altlinux.org>
> > > ---
> > >  hasher-priv/chrootuid.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/hasher-priv/chrootuid.c b/hasher-priv/chrootuid.c
> > > index 89c112e..357d3ef 100644
> > > --- a/hasher-priv/chrootuid.c
> > > +++ b/hasher-priv/chrootuid.c
> > > @@ -134,6 +134,11 @@ chrootuid(uid_t uid, gid_t gid, const char *ehome,
> > >  	/* Set close-on-exec flag on all non-standard descriptors. */
> > >  	cloexec_fds();
> > >  
> > > +	sigset_t sigmask;
> > > +
> > > +	sigemptyset(&sigmask);
> > > +	sigprocmask(SIG_SETMASK, &sigmask, NULL);
> > > +
> > >  	block_signal_handler(SIGCHLD, SIG_BLOCK);
> > >  
> > >  	if ((pid = fork()) < 0)
> > 
> > Assuming it really should reset the signal mask (I don't have the context
> 
> Parent processes use signalfd(2) to handle signals and block those
> signals before opening the signalfd.

The question is what is the correct place to unblock them.

> % git grep -nF 'sigprocmask('                            
> hasher-priv/caller_server.c:236:        sigprocmask(SIG_SETMASK, &mask, NULL);
> hasher-priv/chrootuid.c:140:    sigprocmask(SIG_SETMASK, &sigmask, NULL);
> hasher-priv/hasher-privd.c:315: sigprocmask(SIG_SETMASK, &mask, NULL);
> hasher-priv/signal.c:27:        if (sigprocmask(what, &set, 0) < 0)

sigprocmask has a return value, it must be tested in all cases.

> > to say whether it should or not), looks like it should rather be written as
> > 
> > 	block_signal_handler(SIGCHLD, SIG_SETMASK);
> > 
> > instead of
> > 
> > 	sigset_t sigmask;
> > 	sigemptyset(&sigmask);
> > 	sigprocmask(SIG_SETMASK, &sigmask, NULL);
> > 	block_signal_handler(SIGCHLD, SIG_BLOCK);
> > 
> > ?
> 
> I'd never seen a call like block_signal_handler(*, SIG_SETMASK)
> in hasher-priv codebase at the time + I decided to make the patches as
> non-intrusive to the unchanged part of the codebase as possible.
> That's why I wrote this as is; I don't mind to change it, though.

Well, it's a very unusual approach when you're rewriting the whole thing.


-- 
ldv