From owner-svn-src-head@FreeBSD.ORG Sat Aug 23 18:42:32 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BE625711; Sat, 23 Aug 2014 18:42:32 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 5BFF530D3; Sat, 23 Aug 2014 18:42:32 +0000 (UTC) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id s7NIgLla070102 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 23 Aug 2014 21:42:21 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua s7NIgLla070102 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s7NIgLd8070101; Sat, 23 Aug 2014 21:42:21 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 23 Aug 2014 21:42:21 +0300 From: Konstantin Belousov To: Jilles Tjoelker Subject: Re: svn commit: r265003 - head/secure/usr.sbin/sshd Message-ID: <20140823184221.GV2737@kib.kiev.ua> References: <20140820151310.GB2737@kib.kiev.ua> <53F4BC9B.3090405@FreeBSD.org> <53F4BEB1.6070000@FreeBSD.org> <53F4C022.5050804@FreeBSD.org> <20140821080541.GE2737@kib.kiev.ua> <53F5D42E.9080908@FreeBSD.org> <20140821123246.GH2737@kib.kiev.ua> <20140822134353.GA21891@stack.nl> <20140822153139.GN2737@kib.kiev.ua> <20140823175244.GA29648@stack.nl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="oe8NOCi/1gP69+wf" Content-Disposition: inline In-Reply-To: <20140823175244.GA29648@stack.nl> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Roger Pau Monn? , src-committers@freebsd.org, Bryan Drewery X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Aug 2014 18:42:32 -0000 --oe8NOCi/1gP69+wf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Aug 23, 2014 at 07:52:44PM +0200, Jilles Tjoelker wrote: > On Fri, Aug 22, 2014 at 06:31:39PM +0300, Konstantin Belousov wrote: > > On Fri, Aug 22, 2014 at 03:43:53PM +0200, Jilles Tjoelker wrote: > > > This is good and necessary for SA_SIGINFO (because of the type of the > > > SIG_DFL and SIG_IGN constants, and because POSIX says so in the > > > description of SA_RESETHAND in the sigaction() page). However, there > > > seems no reason to clear the other flags, which have no effect when t= he > > > disposition is SIG_DFL or SIG_IGN but have historically always been > > > preserved. (Slight exception: if kernel code erroneously loops on > > > ERESTART, it will eat CPU time iff SA_RESTART is set, independent of = the > > > signal's disposition.) > > Well, I already committed the patch with several bugs fixed comparing > > with what was mailed, before your feedback arrived. >=20 > > Do you consider it is important enough to revert the resetting of other > > flags ? In particular, your note about the traditional historic > > behaviour makes me wonder. >=20 > I consider it important enough. Clearing the other flags is not > POSIX-compliant and might break applications. For example, I can imagine > an application modifying a struct sigaction with sa_handler =3D=3D SIG_DFL > from a sigaction() call. This feels somewhat strange to me. E.g., I can easily imagine an implementation which relies on some code executing in the process user context for default action on some signal. Having the flags, like SA_ONSTACK or SA_NODEFER to influence the handler is weird. Such implementation is not unix, but I think it is quite possible that cygwin or interix do core dumping in userspace. >=20 > > I do not see why SA_SIGINFO is so special that it must be reset, > > while other flags are not. The absence of the cases where the > > default/ignored disposition is affected by the flags seems rather > > arbitrary. >=20 > The difference is that SA_SIGINFO changes the disposition field from > sa_handler to sa_sigaction, and it is not unambiguously clear how > SIG_DFL and SIG_IGN are represented in sa_sigaction. Note that > sa_handler and sa_sigaction may or may not share storage, and > implementations may or may not support (void (*)(int, siginfo_t *, void > *))SIG_DFL. >=20 > For example, when I wrote system() using posix_spawn() in > https://lists.freebsd.org/pipermail/freebsd-hackers/2012-July/040065.html > I needed to know whether SIGINT and SIGQUIT were ignored or not. I wrote >=20 > ] if ((intact.sa_flags & SA_SIGINFO) !=3D 0 || > ] intact.sa_handler !=3D SIG_IGN) > ] (void)sigaddset(&defmask, SIGINT); > ] if ((quitact.sa_flags & SA_SIGINFO) !=3D 0 || > ] quitact.sa_handler !=3D SIG_IGN) > ] (void)sigaddset(&defmask, SIGQUIT); >=20 > in the assumption that there is always an actual handler if SA_SIGINFO > is set. I did not really like this code and eventually system() was > changed to use vfork() directly instead of posix_spawn(), but I think it > is correct. If the implementation provides separate storage for sa_handler and sa_sigaction, then isn't it more correct to assume that sa_handler is NULL when sa_sigaction is valid ? In other words, simple sa.sa_handler !=3D SIG_IGN test would be more reasonable. I see that the SUSv4 is worded in a way that SA_SIGINFO always accompanies sa_sigaction. Are there any implementations which separate the storage for sa_handler and sa_sigaction ? >=20 > Note that this means that it is sometimes necessary to install a handler > function that will never be called. Per POSIX, SA_SIGINFO must be set > for sigwaitinfo() to be guaranteed to return siginfo_t data, and the > only way to do this is to specify a handler function, even though it > will never be called because the signal is masked. (A never-called > handler function also needs to be specified when using sigwait-like > functions with signals that default to ignore.) >=20 > The other flags do not affect the representation of the disposition, and > can therefore remain set without problem. Anyway, below is the patch with reverts the behaviour WRT flags other than SA_SIGINFO. I like the compactness of sigact_flag_test() calls, so I kept the function, despite it is only used in non-trivial ways for SA_SIGINFO flag test in kern_sigaction(). diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 8810bf3..f73c801 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -625,9 +625,14 @@ static bool sigact_flag_test(struct sigaction *act, int flag) { =20 - return ((act->sa_flags & flag) !=3D 0 && - (__sighandler_t *)act->sa_sigaction !=3D SIG_IGN && - (__sighandler_t *)act->sa_sigaction !=3D SIG_DFL); + /* + * SA_SIGINFO is reset when signal disposition is set to + * ignore or default. Other flags are kept according to user + * settings. + */ + return ((act->sa_flags & flag) !=3D 0 && (flag !=3D SA_SIGINFO || + ((__sighandler_t *)act->sa_sigaction !=3D SIG_IGN && + (__sighandler_t *)act->sa_sigaction !=3D SIG_DFL))); } =20 /* @@ -916,7 +921,6 @@ siginit(p) for (i =3D 1; i <=3D NSIG; i++) { if (sigprop(i) & SA_IGNORE && i !=3D SIGCONT) { SIGADDSET(ps->ps_sigignore, i); - SIGADDSET(ps->ps_sigintr, i); } } mtx_unlock(&ps->ps_mtx); @@ -936,10 +940,6 @@ sigdflt(struct sigacts *ps, int sig) SIGADDSET(ps->ps_sigignore, sig); ps->ps_sigact[_SIG_IDX(sig)] =3D SIG_DFL; SIGDELSET(ps->ps_siginfo, sig); - SIGADDSET(ps->ps_sigintr, sig); - SIGDELSET(ps->ps_sigonstack, sig); - SIGDELSET(ps->ps_sigreset, sig); - SIGDELSET(ps->ps_signodefer, sig); } =20 /* --oe8NOCi/1gP69+wf Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJT+OCNAAoJEJDCuSvBvK1BKcYQAKpxOk+LyLS0MQwyx6auNUT4 XXf66wfIEVvvc8dTdpUmmZBruOcBBytohHbIszT1Zt+vMAmdmzGrqfWdnxong03r 8LYGVftz7FHQfW4ruiJapQSV5X9IToTGhDa9o8PbDj2YEfERKQ6s8EqoItg56fXX sJQIJtwyFwrIAiDUHrEC0SonFoueWjmIgM1hS2diw2Wucd6X8TWPTlCyLHTEfmsb OPsJtqWijMAI6FQfQowaoVLmm8vXL0kM43ubRavByDkodJ2Cf2L1lN8hVuVE6YBH Rkxq6xP4bTB+3TuqcYHoEPHxccX/rCnmU27bChlPp5cxU/qn9fJ+qJnB/u/JwSon 3DhccswpP013tfqGnq5KT+Ni2mBjMWAZKfGYx/bTl2QzjB0Vyb3jhRG8G+q0UZvL 72GuHeRJyrU5ncDOfqmjVkVejf2a0e8urGAzXhD3BiN0HJPQN1oHvXCDwPfVkazr akWgCvYZK3ZPuw6gQzHjZpZ6RQ0bWiVPzpFidOLQITjzih3nPIbbfiiqamq67UVv ngKNXeMyzMgUdDKmG+dP3PQ+FJvHZfIh2yKK7c/6bDAliG4SDwz8C/R4E9ArA7un lh7b5Z7rJNGf7yrFXtU7RfgVmuwrFGHsi0pykQWkFVtblbheMnP5r55L1TFMA4XN ZhUulceDIKFDCkqSnjHA =d2xr -----END PGP SIGNATURE----- --oe8NOCi/1gP69+wf--