If we need an ifdef I think we'd be better off checking for the presence of the thing needed (ie "#ifdef I_FIND").  It's also be nice to have a tidier way of doing this, although I'm not sure there is one...

This issue is fixed in recent illumos and Solaris so that it is not possible to push these modules multiple times. However, adding a check is still a reasonable thing to do in order to support unpatched OS versions since XPG4v2 effectively requires that the OS pushes the modules on opening a slave pseudo terminal device.

First question is why is so important to build according more then 20 (30?) yeas old specification like XPG4?

Using more modern XPG6 (version 2004 - https://pubs.opengroup.org/onlinepubs/009695399/functions/posix_openpt.html) there is requirements for stream modules.

I did some tests using implementation from https://roumenpetrov.info.example.net/secsh/  on Solaris 11.3 and the test shows that stream modules are not inserted:
=====
debug1: Allocating pty.

..TRACE openpty()  ...
..TRACE openpty() I_FIND ptem    : 0
..TRACE openpty() I_FIND ldterm  : 0
..TRACE openpty() I_FIND ttcompat: 0
debug1: session_pty_req: session 0 alloc /dev/pts/5
=====
So there is no need for patched OS!


Just stop to build for XPG4.

Why such build succeeded is another question.

Regards,
Roumen Petrov

This applies in Solaris to XPG4 and above, so XPG6 too. If the modules are not being inserted, then the test code might have been built without XPG4 or 6 - it depends on the how the compiler you are using is configured but did you compile with std=C90 or above?

Look at `gcc -dumpspecs` for the startfile_arch section and if it includes values-xpg4.o or values-xpg6.o then you are compiling to that standard with the right flags, otherwise not.

This is from Solaris 11.3, gcc 7.3

*startfile_arch:
%{ansi:values-Xc.o%s; :values-Xa.o%s}      %{std=c99|std=c9x|std=gnu99|std=gnu9x:values-xpg6.o%s}      %{std=c11|std=c1x|std=gnu11|std=gnu1x:values-xpg6.o%s}      %{std=c++11|std=c++0x|std=gnu++11|std=gnu++0x:values-xpg6.o%s}      %{std=c++14|std=c++1y|std=gnu++14|std=gnu++1y:values-xpg6.o%s}      %{std=c++1z|std=gnu++1z:values-xpg6.o%s}

so to get an xpg6 build, you need at least -std=c99.

Created attachment 3248 [details]
revised diff

This is a simplified patch that avoids making this file more of a #ifdef hell and adopts Darren's suggestion of testing for I_FIND directly.

I'm lacking the ability to test this, so I'd appreciate if someone else could.

Nobody stepped up to test this, so this won't make the openssh-8.0 release. It won't make the 8.1 release either unless someone does.

Sorry, Damien, as the original reporter I should have tested your patch. I will do so on Oracle Solaris 11.4 in addition to an Illumos-based distro just to be on the safe side.

In addition, I am pursuing some really strange behavior with OpenSSH 7.9p1 in OpenIndiana oi151a9 (a really old version) where connecting to OpenSSH, Control-C in a shell causes the connection to freeze, as does Control-G in Emacs, when SunSSH does not exhibit this behavior. I suspect some interaction with the line discipline used by ksh93, tcsh and bash and sshd.

OK, I verified Damien's patch on SmartOS joyent_20170302T081240Z.

I'm having some difficulties building the dependencies on Oracle Solaris 11.4, so please bear with me.

(In reply to Fazal Majid from comment #8)
> In addition, I am pursuing some really strange behavior with OpenSSH
> 7.9p1 in OpenIndiana oi151a9 (a really old version) where connecting
> to OpenSSH, Control-C in a shell causes the connection to freeze

That sounds a bit like the STREAMS pty master bug (see bugzilla #245) wherein signals get delivered to the sshd not the user's shell.  If that's it, you can work around it by defining SSHD_ACQUIRES_CTTY.

Thanks Darren, but that doesn't seem to make a difference.

Here is output of ssh -v -v -v on the client just after issuing the Ctrl-C:

mordac ~>echo debug3: receive packet: type 96
debug2: channel 0: rcvd eof
debug2: channel 0: output open -> drain
debug2: channel 0: obuf empty
debug2: channel 0: close_write
debug2: channel 0: output drain -> closed

and on the server side with sshd -d -d -d:

debug2: channel 0: read<=0 rfd 11 len 0
debug2: channel 0: read failed
debug2: channel 0: chan_shutdown_read (i0 o0 sock -1 wfd 11 efd -1 [closed])
debug2: channel 0: input open -> drain
debug2: channel 0: ibuf empty
debug2: channel 0: send eof
debug3: send packet: type 96
debug2: channel 0: input drain -> closed

so it looks like sshd read 0 bytes and decided to send an EOF to the client. The connection was still open, as was the child process of sshd until I close it with a ~.

It's also worth noting that programs that should put themselves in raw mode like tmux or screen are also killed by a Ctrl-C, so something is off with line disciplines, even though the STREAMS module order seems correct.

(In reply to Fazal Majid from comment #11)
[...] 
> so it looks like sshd read 0 bytes and decided to send an EOF to the
> client.

Ah that's a different problem with a different workaround: try defining PTY_ZEROREAD.

The Control-C problem only occurs with bash and tcsh, not /bin/sh, so it's not an OpenSSH issue per se (I will try your PTY_ZEROREAD workaround).

Going back to testing Damien's patch on Oracle Solaris 11.4, sorry for the diversion.

PTY_ZEROREAD does stop the hanging, but screen and tmux remain in semi-raw line discipline where Control-C kills them with bash and tcsh.

Successfully tested on Oracle Solaris 11.4 (without SSHD_ACQUIRES_CTTY or PTY_ZEROREAD).

No duplicate STREAMS modules, and Control-C works properly in tcsh.

Comment on attachment 3248 [details]
revised diff

https://bugzilla.mindrot.org/show_bug.cgi?id=2998#c24 also confirms this seems to work.

I've committed the patch and it'll be in the next release. Thanks.

closing resolved bugs as of 8.6p1 release