svn commit: r265003 - head/secure/usr.sbin/sshd

[Konstantin Belousov]

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 libs.
> >>>>>   Due to the change in ld(1) behaviour of not copying NEEDED entries
> >>>>>   from direct dependencies into the link results, the order becomes
> >>>>>   reversed.
> >>>>>   
> >>>>>   The libthr must appear before libc to properly interpose libc symbols
> >>>>>   and provide working rtld locks implementation.  The symptom was sshd
> >>>>>   hanging on rtld bind lock during nested symbol binding from a signal
> >>>>>   handler.
> >>>>>   
> >>>>>   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
> >>>>> ==============================================================================
> >>>>> --- head/secure/usr.sbin/sshd/Makefile	Sun Apr 27 05:19:01 2014	(r265002)
> >>>>> +++ head/secure/usr.sbin/sshd/Makefile	Sun Apr 27 05:28:14 2014	(r265003)
> >>>>> @@ -57,6 +57,16 @@ CFLAGS+= -DNONE_CIPHER_ENABLED
> >>>>>  DPADD+= ${LIBCRYPT} ${LIBCRYPTO} ${LIBZ}
> >>>>>  LDADD+= -lcrypt -lcrypto -lz
> >>>>>  
> >>>>> +# 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 before
> >>>>> +# the -lc added by a compiler driver.
> >>>>> +.if ${MK_KERBEROS_SUPPORT} != "no"
> >>>>> +DPADD+= ${LIBPTHREAD}
> >>>>> +LDADD+= -lpthread
> >>>>> +.endif
> >>>>> +
> >>>>>  .if defined(LOCALBASE)
> >>>>>  CFLAGS+= -DXAUTH_PATH=\"${LOCALBASE}/bin/xauth\"
> >>>>>  .endif
> >>>>
> >>>> Hello,
> >>>>
> >>>> This change makes the following simple test program fail on the second 
> >>>> assert. The problem is that sa_handler == SIG_DFL, and sa_flags == 
> >>>> SA_SIGINFO, which according to the sigaction(9) man page is not 
> >>>> 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.
> >>
> > 
> > Does it fail if you do not connect with ssh?
> 
> Right, it works fine from the serial console, fails when executed from ssh.

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);
 }
 
+static bool
+sigact_flag_test(struct sigaction *act, int flag)
+{
+
+	return ((act->sa_flags & flag) != 0 &&
+	    (__sighandler_t *)act->sa_sigaction != SIG_IGN &&
+	    (__sighandler_t *)act->sa_sigaction != SIG_DFL);
+}
+
 /*
  * kern_sigaction
  * sigaction
@@ -679,7 +688,7 @@ kern_sigaction(td, sig, act, oact, flags)
 
 		ps->ps_catchmask[_SIG_IDX(sig)] = 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)] =
 			    (__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)] = 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);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20140821/d91eeb63/attachment.sig>