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

[Arseny Maslennikov <arseny@altlinux.org>]

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

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.

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

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

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