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

[Roger Pau Monné]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 21/08/14 10:05, Konstantin Belousov wrote:
> 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 ?

IMO, it is an inconsistency to return an invalid old sigaction, I
assume that what is returned as the old sigaction should also be valid
according to the man page.

I realize SUSv4 don't specify such requirement, but it would still be
wrong to use SIG_DFL with SA_SIGINFO, since SA_SIGINFO expect the
handler to be of the type:

void func(int signo, siginfo_t *info, void *context);

While SIG_DLF is of type:

void func(int signo);

There's software out there that (wrongly?) relies on sa_action ==
SIG_DFL and (sa_flags & SA_SIGINFO) == 0:

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_fork.c;h=fa150959adcfa6618342ba1eb0085cbba5f75d0a;hb=HEAD#l338

The sa_flags check done here seems too strong in my opinion, but I
still think it's right according to the man page.

Roger.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Darwin)

iQEcBAEBAgAGBQJT9dQpAAoJEKXZdqUyumTAgsIH/2xAfa0FjWpCpkvDoNXGVs4K
tRDurCFTsaCJZ1xt3aQyPvPALm+qOpBX+i3nTiX4Bg86jbrZRGTag4OeAE6uX3KR
TCKaUB6jNUjuNsj5djURIQktbojFj71ID40bM3AXExXN8Gc7e9qqdvo+p82hDFS/
RkwwS9NfTv+yeC/djH+PsApq7OYCrpR0CX1fW6TKwtjdEZpJC4jx5S5TVJoZ2Y0B
urlCtrjW6b4oNHqoiDMF4nk48SkuU/JWsTGAbFW6lK+1voyt3y1126uFk5jz144M
ZYy4fu6mKEddrwrUFD9Qt9r3shaSLbenBxhc2ZxMT9V4Ws87bVxTSqqzrYsHJ0E=
=Qwfa
-----END PGP SIGNATURE-----