fix(proc_open): fix pty being on the same fd

[joelwurtz]

This fix #17983

It's hard to do a reproducer that correspond exactly to the test.

However now the behavior is that reading on the input pipe will not give the stdout output (see the modified test)

I can make a test for this behavior (so it ensure fd are not shared)

[joelwurtz]

Friendly ping @alexdowad do you remember the reason it use the same pty ?

I did look at #5525 and seeing last @nikic comment seems to be the issue (that's why i split them)

But maybe i'm missing something ?

[bukka]

If no one is quicker, I will hopefully take a look later this week.

[alexdowad]

@joelwurtz If you study the GH thread where I re-enabled support for PTY in proc_open in 2020, you will see that this support was originally added many years before (apparently 16 years?) but had been disabled with #if 0 shortly after that.

The original implementer who added PTY support around 2004 was @wez. My implementation just simplified and refactored his code a bit. But I didn't change the behavior whereby multiple 'pty' pipes all share the same FD. I really doubt that @wez will remember why his code worked that way, but if he has something to say, maybe he will speak up.

While the original implementation from 2004 was disabled in php-src, some packagers (including the package for Debian) had re-enabled it by patching out #if 0. So if the behavior is changed now, there is the potential for breaking 20 years worth of user code which has accumulated since then.

I'm not the primary maintainer of this code, and I won't hazard an opinion on whether the behavior should be changed now or not. You can discuss that with other maintainers. I will just say that working on PHP has taught me again and again the truth of Hyrum's Law. Especially with a heavily-used project like PHP, it is very, very hard to change existing behavior without breaking a lot of people's code. For that reason, if others feel that this change is a good idea, it might be good to go through the RFC process. Also, definitely NEWS and UPGRADING should both be updated.

Otherwise, thanks for reporting the issue you are having and for sharing your patch.

[wez]

I don't recall the details around those changes from so long ago, but after peeking at #17983, I wonder if the run_process php function in the reproducer itself is logically "wrong": a PTY doesn't have stdout vs. stderr, it's just a single output stream. I think it is logically an error to try to read and combine both stdout and stderr from proc open when it is associated with a pty, and that you should probably just pick one and close the other out if you don't need it.

I would be hesitant to change the behavior of proc_open to try to compensate for this because it is pretty much a certainty that someone is relying on how it works today.

[joelwurtz]

I would be hesitant to change the behavior of proc_open to try to compensate for this because it is pretty much a certainty that someone is relying on how it works today.

That is definitely possible (even the current test rely on this behavior as it read stdout from the stdin pipe)

Having the same pty for stdin / stdout shoult not be a problem, but it is when there is stderr

I also don't know why it works in most cases actually, this error happens randomly and it seems to occurs when a pipe is not ready, maybe the kernel write to the first one available in this case ?

There is also the possibility to have a second parameter, that tell to force the creation of a new pty :

$descriptors = [['pty', 'pty', ['pty', true]]; 

I think that would allow this behavior while still preserving BC compatibility

[alexdowad]

There is also the possibility to have a second parameter, that tell to force the creation of a new pty :

$descriptors = [['pty', 'pty', ['pty', true]]; 

I think that would allow this behavior while still preserving BC compatibility

Suggest again that an RFC would be a good way to propose this new feature.

[wez]

but it is when there is stderr

but there is no stderr in a PTY, there is a single output stream that has been aliased to both the stdout and stderr handles in what is returned from proc_open.

Reading from those will read from the underlying PTY output stream.

It does not make a whole lot of sense to try to read from both to combine them; you should just read from one or the other.

If you need separate streams, end to end, you should use pipes instead of a PTY, or maybe a PTY for stdout and a pipe for stderr.

I think trying to spawn multiple PTYs for this sort of thing is almost certainly not what you want to do. I don't think I've ever seen such a thing before.

[joelwurtz]

If you need separate streams, end to end, you should use pipes instead of a PTY, or maybe a PTY for stdout and a pipe for stderr.

Goal of using PTY is to pass a stream that is connected to terminal (isatty -> true) for an underlying process while still having the possibility to parse the output from PHP (we run multiple process in parallel so we need to correctly split output for end user).

I think most of our use cases involve passing an stdout tty compatible stream (so pty) to the underlying process, stderr not being tty compatible is not a problem generally, however there may be some cases when it would be nice to also pass a pty for stderr (if the underlying process have a different behavior when stderr is tty, like coloration or anything else)

Also symfony use ['pty', 'pty', 'pty'] by default when we want this behavior (will create a PR there to check if we could have a "degraded" mode : stdout + stdin = pty and stderr being a pipe, or even better have way to directly specific descriptors, so we could using correct one given the current state)

[alexdowad]

Goal of using PTY is to pass a stream that is connected to terminal (isatty -> true) for an underlying process while still having the possibility to parse the output from PHP (we run multiple process in parallel so we need to correctly split output for end user).

I think most of our use cases involve passing an stdout tty compatible stream (so pty) to the underlying process, stderr not being tty compatible is not a problem generally, however there may be some cases when it would be nice to also pass a pty for stderr (if the underlying process have a different behavior when stderr is tty, like coloration or anything else)

@joelwurtz If you think of how CLI programs normally run on Unix-like systems, the usual case is that stdin and stderr are both printed out at the same terminal. The terminal does not distinguish text which was written via the stdin stream and that written via the stderr stream in any way; it just prints everything out together. This is the same as the current behavior of proc_open, whereby stdout and stderr refer to the same PTY.

Your desired behavior, where the stdout and stderr streams are set to different PTYs, is as if a CLI program is invoked from a shell, and then the shell causes two different terminal windows to pop up, one of them displaying stdout and the other displaying stderr. It does without saying that this would be very strange and surprising behavior.

The usual solution employed by users of Unix-like systems, is that when one needs to distinguish stdout and stderr, they pipe stderr to somewhere other than the terminal, using the 2>somewhere shell syntax. This is exactly the same as what @wez has encouraged you to do by using 'pty' for stdout and a pipe for stderr. As you said, this does mean that isatty will return false for stderr, but Unix users in general haven't seemed to have much of a problem with that over the last ~50 years.

I think maybe this PR can be closed.

[joelwurtz]

I also agree with that, let met gather feedback on symfony/symfony#59949 where i have made the change so if there is other reason why it may need that i can report them back here.