Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #69442: Implemented fix for incorrect filedescriptor closing #1588

Closed
wants to merge 1 commit into from

Conversation

jaytaph
Copy link
Contributor

@jaytaph jaytaph commented Oct 21, 2015

This PR fixes issue 69442, which was discovered through some functionality within the Symfony2 framework: symfony/symfony#12643

Basically, it only manifests itself when PTS is enabled. This isn't the case in a standard PHP distribution, however, Ubuntu/Debian SRPM adds a patch to enabled this functionality (note that the patch itself does not include the functionality, it only ENABLES the already present functionality).

I've added a PHPT test, as created initially by @ewgRa (some additional effort has been taken to skip the test if PTY is not supported, i'm sure there are better ways to achieve the same goal in a less fragile way).

@krakjoe
Copy link
Member

krakjoe commented Jan 6, 2017

Merged ff6b309 69b1526 0ca4358

Thanks :)

@krakjoe krakjoe closed this Jan 6, 2017
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 4, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Could this mean that on the platform where this test case was originally developed,
the PHP parent process ran *faster* than the child process, so that all the calls to
fread() complete before the child process exits and closes its FDs? Or could something
else about the platform have been different, such that read() in the parent does not fail
with EIO even after the child process exits? Another question: Are the Debian packagers
even running this test case every time they build PHP binaries?

Intriguing questions, these!

This voluminous commit message would be incomplete without one more point: IF at least
one FD referring to the slave end of the PTY is kept open *in the parent process*, the
failure with EIO will not occur even after the child process exits. This might be a good
approach, though we need a way to ensure the FD will be closed eventually in long-running
programs. Perhaps associate it with the proc resource and close it when the proc is freed
or explicitly closed with proc_close?
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 4, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Could this mean that on the platform where this test case was originally developed,
the PHP parent process ran *faster* than the child process, so that all the calls to
fread() complete before the child process exits and closes its FDs? Or could something
else about the platform have been different, such that read() in the parent does not fail
with EIO even after the child process exits? Another question: Are the Debian packagers
even running this test case every time they build PHP binaries? Intriguing questions, these!

There may be another way out of this sticky dilemma: IF at least one FD referring to the
slave end of the PTY is kept open *in the parent process*, the failure with EIO will not
occur even after the child process exits. This might be a good approach, though we would
need a way to ensure the FD will be closed eventually in long-running programs. Perhaps
associate it with the proc resource and close it when the proc is freed or explicitly closed
with proc_close?

And the rabbit hole goes deeper. Although (for now) the test case from 2015 has been
updated so it does not attempt to fread() multiple times from the PTY, it still fails
intermittently. The reason? When the child process writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). The "\r" is obviously
from the TTY line discipline converting "\n" to "\r\n", but why on earth would the parent
sometimes read the newline and sometimes not? What is more, strace clearly shows that this
is happening *in the kernel*. The child process always executes a single write("foo\n")
syscall, but when the blocking read() syscall issued by the parent process returns,
sometimes it returns "foo" and sometimes "foo\r\n". This doesn't happen when I extract the
test code and run it directly from the PHP CLI, only when the test case is run by
run-tests.php.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 4, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Could this mean that on the platform where this test case was originally developed,
the PHP parent process ran *faster* than the child process, so that all the calls to
fread() complete before the child process exits and closes its FDs? Or could something
else about the platform have been different, such that read() in the parent does not fail
with EIO even after the child process exits? Another question: Are the Debian packagers
even running this test case every time they build PHP binaries? Intriguing questions, these!

There may be another way out of this sticky dilemma: IF at least one FD referring to the
slave end of the PTY is kept open *in the parent process*, the failure with EIO will not
occur even after the child process exits. This might be a good approach, though we would
need a way to ensure the FD will be closed eventually in long-running programs. Perhaps
associate it with the proc resource and close it when the proc is freed or explicitly closed
with proc_close?

And the rabbit hole goes deeper. Although (for now) the test case from 2015 has been
updated so it does not attempt to fread() multiple times from the PTY, it still fails
intermittently. The reason? When the child process writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). The "\r" is obviously
from the TTY line discipline converting "\n" to "\r\n", but why on earth would the parent
sometimes read the newline and sometimes not? What is more, strace clearly shows that this
is happening *in the kernel*. The child process always executes a single write("foo\n")
syscall, but when the blocking read() syscall issued by the parent process returns,
sometimes it returns "foo" and sometimes "foo\r\n". This doesn't happen when I extract the
test code and run it directly from the PHP CLI, only when the test case is run by
run-tests.php.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 7, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Likely this is because PHP's fread() was updated to report errors from the underlying
system calls only recently.

One way out of this dilemma: IF at least one FD referring to the slave end of the PTY is
kept open *in the parent process*, the failure with EIO will not occur even after the child
process exits. However, that would raise another issue: we would need a way to ensure the FD
will be closed eventually in long-running programs.

Another discovery made while testing this code is that fread() does not always return
all the data written to the slave end of the PTY in a single call, even if the data was
written with a single syscall and it is only a few bytes long.

Specifically, when the child process in the test case writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). (The "\r" is from the
TTY line discipline converting "\n" to "\r\n".) A second call to fread() does return the
remaining bytes, though sometimes all the data is read in the first call, and by the time
the second call is made, the child process has already exited. It seems that liberal use
of the @ operator is needed when using fread() on pipes.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 7, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Likely this is because PHP's fread() was updated to report errors from the underlying
system calls only recently.

One way out of this dilemma: IF at least one FD referring to the slave end of the PTY is
kept open *in the parent process*, the failure with EIO will not occur even after the child
process exits. However, that would raise another issue: we would need a way to ensure the FD
will be closed eventually in long-running programs.

Another discovery made while testing this code is that fread() does not always return
all the data written to the slave end of the PTY in a single call, even if the data was
written with a single syscall and it is only a few bytes long.

Specifically, when the child process in the test case writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). (The "\r" is from the
TTY line discipline converting "\n" to "\r\n".) A second call to fread() does return the
remaining bytes, though sometimes all the data is read in the first call, and by the time
the second call is made, the child process has already exited. It seems that liberal use
of the @ operator is needed when using fread() on pipes.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 12, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Likely this is because PHP's fread() was updated to report errors from the underlying
system calls only recently.

One way out of this dilemma: IF at least one FD referring to the slave end of the PTY is
kept open *in the parent process*, the failure with EIO will not occur even after the child
process exits. However, that would raise another issue: we would need a way to ensure the FD
will be closed eventually in long-running programs.

Another discovery made while testing this code is that fread() does not always return
all the data written to the slave end of the PTY in a single call, even if the data was
written with a single syscall and it is only a few bytes long.

Specifically, when the child process in the test case writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). (The "\r" is from the
TTY line discipline converting "\n" to "\r\n".) A second call to fread() does return the
remaining bytes, though sometimes all the data is read in the first call, and by the time
the second call is made, the child process has already exited. It seems that liberal use
of the @ operator is needed when using fread() on pipes.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
alexdowad added a commit to alexdowad/php-src that referenced this pull request May 13, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR php#1588
(php#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Likely this is because PHP's fread() was updated to report errors from the underlying
system calls only recently.

One way out of this dilemma: IF at least one FD referring to the slave end of the PTY is
kept open *in the parent process*, the failure with EIO will not occur even after the child
process exits. However, that would raise another issue: we would need a way to ensure the FD
will be closed eventually in long-running programs.

Another discovery made while testing this code is that fread() does not always return
all the data written to the slave end of the PTY in a single call, even if the data was
written with a single syscall and it is only a few bytes long.

Specifically, when the child process in the test case writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). (The "\r" is from the
TTY line discipline converting "\n" to "\r\n".) A second call to fread() does return the
remaining bytes, though sometimes all the data is read in the first call, and by the time
the second call is made, the child process has already exited. It seems that liberal use
of the @ operator is needed when using fread() on pipes.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
php-pulls pushed a commit that referenced this pull request May 14, 2020
Back in 2004, a feature was added to proc_open which allowed it to open a PTY,
connecting specific FDs in the child process to the slave end of the PTY and returning
the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However,
this feature was disabled just about a month later. Little information is available
about why this was done, but from talking to the original implementer, it seems there
were portability problems with some rare flavors of Unix.

Re-enable this feature with a simplified implementation which uses openpty(). No
attempt is made to support PTYs if the platform does not have openpty(). The configure
script checks if linking with -lutil is necessary to use openpty(), but if anything
else is required, like including some special header or linking with some other library,
PTY support will be disabled.

The original PTY support for proc_open automatically daemonized the child process
(disassociating it from the TTY session and process group of the parent). However,
I don't think this is a good idea. Just because a user opens a child process in a
PTY, it doesn't mean they want it to continue running even when the parent process
is killed. Of course, if the child process is some kind of server, it will likely
daemonize itself; but we have no reason to preempt that decision.

It turns out that since 2015, there has been one test case for PTY support in
proc_open() in the test suite. This test was added in GitHub PR #1588
(#1588). That PR mentioned that the PHP
binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking
the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this
is still true. Debian's patch does not modify the implementation from 2004 in any
way; it just removes the #if 0 line which disables it.

Naturally, the test case is skipped if PTY support is not enabled. This means that ever
since it was added, every test run against the 'vanilla' PHP codebase has skipped it.

Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both
with this simplified implementation *and* when enabling the original implementation.
Investigation reveals the reason: when the child process using the slave end of the
PTY exits and its FDs are all closed, and all buffered data is read from the master
end of the PTY, any further attempt to read from the master end fails with EIO. The
test case seems to expect that reading from the master end will always return an
empty string if no data is available.

Likely this is because PHP's fread() was updated to report errors from the underlying
system calls only recently.

One way out of this dilemma: IF at least one FD referring to the slave end of the PTY is
kept open *in the parent process*, the failure with EIO will not occur even after the child
process exits. However, that would raise another issue: we would need a way to ensure the FD
will be closed eventually in long-running programs.

Another discovery made while testing this code is that fread() does not always return
all the data written to the slave end of the PTY in a single call, even if the data was
written with a single syscall and it is only a few bytes long.

Specifically, when the child process in the test case writes "foo\n" to the PTY, the parent
sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). (The "\r" is from the
TTY line discipline converting "\n" to "\r\n".) A second call to fread() does return the
remaining bytes, though sometimes all the data is read in the first call, and by the time
the second call is made, the child process has already exited. It seems that liberal use
of the @ operator is needed when using fread() on pipes.

Thanks to Nikita Popov for suggesting that we should just use openpty() rather than
grantpt(), unlockpt(), etc.
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Oct 29, 2023
…ality is enabled (a.dmitryuk)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Process] remove fixing of legacy bug, when PTS functionality is enabled

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        |
| License       | MIT

The PR drop fixing PHP bug, that was closed in PHP 7.0

Original issues:
php/php-src#1588
#16574

Original commit:
php/php-src@ff6b309

Commits
-------

03590f1 [Process] remove fixing of legacy bug, when PTS functionality is enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants