From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 3 Dec 2021 19:06:46 +0300 From: "Dmitry V. Levin" To: devel@lists.altlinux.org Message-ID: <20211203160646.GA14212@altlinux.org> References: <20210824082436.1555890-1-arseny@altlinux.org> <20210824082436.1555890-4-arseny@altlinux.org> <20211201192336.GA12855@altlinux.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [devel] [PATCH hasher-priv v3 3/7] chrootuid: explicitly reset signal mask before forking off payload X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Dec 2021 16:06:46 -0000 Archived-At: List-Archive: List-Post: 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 > > > --- > > > 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