Date: Thu, 21 Aug 2014 11:05:41 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Roger Pau Monn? <royger@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bryan Drewery <bdrewery@FreeBSD.org> Subject: Re: svn commit: r265003 - head/secure/usr.sbin/sshd Message-ID: <20140821080541.GE2737@kib.kiev.ua> In-Reply-To: <53F4C022.5050804@FreeBSD.org> References: <201404270528.s3R5SEIm054377@svn.freebsd.org> <53F4B381.5010205@FreeBSD.org> <20140820151310.GB2737@kib.kiev.ua> <53F4BC9B.3090405@FreeBSD.org> <53F4BEB1.6070000@FreeBSD.org> <53F4C022.5050804@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--/3zO87W6OPidGKTZ Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Wed, Aug 20, 2014 at 05:34:58PM +0200, Roger Pau Monn? wrote: > On 20/08/14 17:28, Bryan Drewery wrote: > > On 8/20/2014 10:19 AM, Roger Pau Monn? wrote: > >> On 20/08/14 17:13, Konstantin Belousov wrote: > >>> On Wed, Aug 20, 2014 at 04:41:05PM +0200, Roger Pau Monn?? wrote: > >>>> On 27/04/14 07:28, Konstantin Belousov wrote: > >>>>> Author: kib > >>>>> Date: Sun Apr 27 05:28:14 2014 > >>>>> New Revision: 265003 > >>>>> URL: http://svnweb.freebsd.org/changeset/base/265003 > >>>>> > >>>>> Log: > >>>>> Fix order of libthr and libc in the global dso list for sshd, by > >>>>> explicitely linking main binary with -lpthread. Before, libthr > >>>>> appeared in the list due to dependency of one of the kerberos lib= s. > >>>>> Due to the change in ld(1) behaviour of not copying NEEDED entries > >>>>> from direct dependencies into the link results, the order becomes > >>>>> reversed. > >>>>> =20 > >>>>> The libthr must appear before libc to properly interpose libc sym= bols > >>>>> and provide working rtld locks implementation. The symptom was s= shd > >>>>> hanging on rtld bind lock during nested symbol binding from a sig= nal > >>>>> handler. > >>>>> =20 > >>>>> Approved by: des (openssh maintainer) > >>>>> Sponsored by: The FreeBSD Foundation > >>>>> MFC after: 1 week > >>>>> > >>>>> Modified: > >>>>> head/secure/usr.sbin/sshd/Makefile > >>>>> > >>>>> Modified: head/secure/usr.sbin/sshd/Makefile > >>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D > >>>>> --- head/secure/usr.sbin/sshd/Makefile Sun Apr 27 05:19:01 2014 (r2= 65002) > >>>>> +++ head/secure/usr.sbin/sshd/Makefile Sun Apr 27 05:28:14 2014 (r2= 65003) > >>>>> @@ -57,6 +57,16 @@ CFLAGS+=3D -DNONE_CIPHER_ENABLED > >>>>> DPADD+=3D ${LIBCRYPT} ${LIBCRYPTO} ${LIBZ} > >>>>> LDADD+=3D -lcrypt -lcrypto -lz > >>>>> =20 > >>>>> +# Fix the order of NEEDED entries for libthr and libc. The libthr > >>>>> +# needs to interpose libc symbols, leaving the libthr loading as > >>>>> +# dependency of krb causes reversed order and broken interposing. = Put > >>>>> +# the threading library last on the linker command line, just befo= re > >>>>> +# the -lc added by a compiler driver. > >>>>> +.if ${MK_KERBEROS_SUPPORT} !=3D "no" > >>>>> +DPADD+=3D ${LIBPTHREAD} > >>>>> +LDADD+=3D -lpthread > >>>>> +.endif > >>>>> + > >>>>> .if defined(LOCALBASE) > >>>>> CFLAGS+=3D -DXAUTH_PATH=3D\"${LOCALBASE}/bin/xauth\" > >>>>> .endif > >>>> > >>>> Hello, > >>>> > >>>> This change makes the following simple test program fail on the seco= nd=20 > >>>> assert. The problem is that sa_handler =3D=3D SIG_DFL, and sa_flags = =3D=3D=20 > >>>> SA_SIGINFO, which according to the sigaction(9) man page is not=20 > >>>> possible. With this change reverted the test is successful. > >>> I do not quite follow. > >>> > >>> What are the relations between sshd and your test program ? > >>> Should the test be run somehow specially ? > >> > >> No, and frankly that's what I don't understand. I compile this simple > >> test with `cc -o test test.c`. It fails with this commit applied, and > >> succeeds without it. > >> > >> Roger. > >> > >=20 > > Does it fail if you do not connect with ssh? >=20 > Right, it works fine from the serial console, fails when executed from ss= h. I cannot reproduce it locally with your scenario, but the attached program demonstrates the issue without relying on inheritance and libthr. I think you mis-interpret the man page statement, it only says that SA_SIGINFO should not be set in new->sa_flags IMO. But I do not see much sense in the requirement. Note that we do not test flags for correctness at all. SUSv4 is also silent on the issue. If this is important for your case, the following patch prevents leaking of the flags for ignored of default/action signals. Could you, please, describe why do you consider this a bug ? diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 561ea0a..3409875 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -621,6 +621,15 @@ sig_ffs(sigset_t *set) return (0); } =20 +static bool +sigact_flag_test(struct sigaction *act, int flag) +{ + + return ((act->sa_flags & flag) !=3D 0 && + (__sighandler_t *)act->sa_sigaction !=3D SIG_IGN && + (__sighandler_t *)act->sa_sigaction !=3D SIG_DFL); +} + /* * kern_sigaction * sigaction @@ -679,7 +688,7 @@ kern_sigaction(td, sig, act, oact, flags) =20 ps->ps_catchmask[_SIG_IDX(sig)] =3D act->sa_mask; SIG_CANTMASK(ps->ps_catchmask[_SIG_IDX(sig)]); - if (act->sa_flags & SA_SIGINFO) { + if (sigact_flag_test(act, SA_SIGINFO)) { ps->ps_sigact[_SIG_IDX(sig)] =3D (__sighandler_t *)act->sa_sigaction; SIGADDSET(ps->ps_siginfo, sig); @@ -687,19 +696,19 @@ kern_sigaction(td, sig, act, oact, flags) ps->ps_sigact[_SIG_IDX(sig)] =3D act->sa_handler; SIGDELSET(ps->ps_siginfo, sig); } - if (!(act->sa_flags & SA_RESTART)) + if (sigact_flag_test(act, SA_RESTART)) SIGADDSET(ps->ps_sigintr, sig); else SIGDELSET(ps->ps_sigintr, sig); - if (act->sa_flags & SA_ONSTACK) + if (sigact_flag_test(act, SA_ONSTACK)) SIGADDSET(ps->ps_sigonstack, sig); else SIGDELSET(ps->ps_sigonstack, sig); - if (act->sa_flags & SA_RESETHAND) + if (sigact_flag_test(act, SA_RESETHAND)) SIGADDSET(ps->ps_sigreset, sig); else SIGDELSET(ps->ps_sigreset, sig); - if (act->sa_flags & SA_NODEFER) + if (sigact_flag_test(act, SA_NODEFER)) SIGADDSET(ps->ps_signodefer, sig); else SIGDELSET(ps->ps_signodefer, sig); --/3zO87W6OPidGKTZ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJT9ahUAAoJEJDCuSvBvK1BPNYP/0k+vHv46B3lSPdiOz7Kx3dA ADJSl0gRQlGEy39cs6cEcUx9ctSw2qaGxFZrrWIrK9NATOoG+twcF1E9DmyUZoXS Zm9c/rHj+OQpqIC1SgF+yvj5fkWoAYTEBBklrrEb0FOwNaTrbRMynwyedGuxloQO VFp9KnP1lIzZZWBRe3PbXkZyK7zOKUrBAQpZzI8goNJdAqsuTDyaj5aNhbhT4J64 VTb0HmVLvuRAUppZNEywFc6tD+TyUwoGLprRhi48awmQhTW/2VIelySUWEzrN4Ar vVpRTzEOavIguNxi9qyqo71BxPGvpd5qyLFStlTgINU9ddp0fCTkZvrUrbYRwnWL DZlGZ1q89mVHhNIe3Qy/pHJstwcgFP4dirZ0KuhbEixrIdaVY9lyB4WZqpMTkqGO OoqZ+jiin4tWkBE2hny7PkJExqNHDO5PUQnV6FNUGOr9zvuQPhCrpPzu2PkSddW9 SYBKvhjcvd47tERemqw2WjhU0SHkoqhm2ghZNiFMq03gjDAiIH4qdT+Vdm8q1aCR Qj0IQhRZCnd8I1ZSlHPbFS9i87NzySWht33HQW0o1yUsQ404T9q2fZREKaGM31+7 7XXhoE4cbW/mVGi7qw8KPtHz7DTE8lW3zXOlijrG2CHKw8ACrxSlX7qeD7YCxOZN q26mSuPhvKxYYqHHgmrW =trCK -----END PGP SIGNATURE----- --/3zO87W6OPidGKTZ--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140821080541.GE2737>