Add PTY support to proc_open (again after 16 long years)

alexdowad · nikic · commit a84cd96e865d · 2020-05-14T10:25:37.000+02:00
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.
diff --git a/configure.ac b/configure.ac
@@ -392,6 +392,7 @@ malloc.h \
 monetary.h \
 netdb.h \
 poll.h \
+pty.h \
 pwd.h \
 resolv.h \
 strings.h \
@@ -560,7 +561,6 @@ getgrnam_r \
 getpwuid_r \
 getwd \
 glob \
-grantpt \
 inet_ntoa \
 inet_ntop \
 inet_pton \
@@ -572,7 +572,6 @@ mmap \
 nice \
 nl_langinfo \
 poll \
-ptsname \
 putenv \
 scandir \
 setitimer \
@@ -588,7 +587,6 @@ strptime \
 strtok_r \
 symlink \
 tzset \
-unlockpt \
 unsetenv \
 usleep \
 utime \
@@ -713,6 +711,9 @@ if test "$PHP_VALGRIND" != "no"; then
   fi
 fi
 
+dnl Check for openpty. It may require linking against libutil.
+PHP_CHECK_FUNC(openpty, util)
+
 dnl General settings.
 dnl ----------------------------------------------------------------------------
 PHP_CONFIGURE_PART(General settings)
diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c
@@ -43,10 +43,13 @@
  * */
 #ifdef PHP_CAN_SUPPORT_PROC_OPEN
 
-#if 0 && HAVE_PTSNAME && HAVE_GRANTPT && HAVE_UNLOCKPT && HAVE_SYS_IOCTL_H && HAVE_TERMIOS_H
-# include <sys/ioctl.h>
-# include <termios.h>
-# define PHP_CAN_DO_PTS	1
+#if HAVE_OPENPTY
+# if HAVE_PTY_H
+#  include <pty.h>
+# else
+/* Mac OS X defines openpty() in <util.h> */
+#  include <util.h>
+# endif
 #endif
 
 #include "proc_open.h"
@@ -592,6 +595,27 @@ static int set_proc_descriptor_to_blackhole(struct php_proc_open_descriptor_item
 	return SUCCESS;
 }
 
+static int set_proc_descriptor_to_pty(struct php_proc_open_descriptor_item *desc, int *master_fd, int *slave_fd)
+{
+#if HAVE_OPENPTY
+	if (*master_fd == -1) {
+		if (openpty(master_fd, slave_fd, NULL, NULL, NULL)) {
+			php_error_docref(NULL, E_WARNING, "Could not open PTY (pseudoterminal) - %s", strerror(errno));
+			return FAILURE;
+		}
+	}
+
+	desc->is_pipe    = 1;
+	desc->childend   = dup(*slave_fd);
+	desc->parentend  = dup(*master_fd);
+	desc->mode_flags = O_RDWR;
+	return SUCCESS;
+#else
+	php_error_docref(NULL, E_WARNING, "PTY (pseudoterminal) not supported on this system");
+	return FAILURE;
+#endif
+}
+
 static int set_proc_descriptor_to_pipe(struct php_proc_open_descriptor_item *desc, zend_string *zmode)
 {
 	php_file_descriptor_t newpipe[2];
@@ -708,9 +732,9 @@ static int redirect_proc_descriptor(struct php_proc_open_descriptor_item *desc,
 	return dup_proc_descriptor(redirect_to, &desc->childend, nindex);
 }
 
-
 int set_proc_descriptor_from_array(
-		zval *descitem, struct php_proc_open_descriptor_item *descriptors, int ndesc, int nindex) {
+		zval *descitem, struct php_proc_open_descriptor_item *descriptors, int ndesc,
+		int nindex, int *pty_master_fd, int *pty_slave_fd) {
 	zend_string *ztype = get_string_parameter(descitem, 0, "handle qualifier");
 	if (!ztype) {
 		return FAILURE;
@@ -749,32 +773,7 @@ int set_proc_descriptor_from_array(
 	} else if (zend_string_equals_literal(ztype, "null")) {
 		retval = set_proc_descriptor_to_blackhole(&descriptors[ndesc]);
 	} else if (zend_string_equals_literal(ztype, "pty")) {
-#if PHP_CAN_DO_PTS
-		if (dev_ptmx == -1) {
-			/* open things up */
-			dev_ptmx = open("/dev/ptmx", O_RDWR);
-			if (dev_ptmx == -1) {
-				php_error_docref(NULL, E_WARNING, "Failed to open /dev/ptmx, errno %d", errno);
-				goto finish;
-			}
-			grantpt(dev_ptmx);
-			unlockpt(dev_ptmx);
-			slave_pty = open(ptsname(dev_ptmx), O_RDWR);
-
-			if (slave_pty == -1) {
-				php_error_docref(NULL, E_WARNING, "Failed to open slave pty, errno %d", errno);
-				goto finish;
-			}
-		}
-		descriptors[ndesc].is_pipe = 1;
-		descriptors[ndesc].childend = dup(slave_pty);
-		descriptors[ndesc].parentend = dup(dev_ptmx);
-		descriptors[ndesc].mode_flags = O_RDWR;
-		retval = SUCCESS;
-#else
-		php_error_docref(NULL, E_WARNING, "PTY pseudo terminal not supported on this system");
-		goto finish;
-#endif
+		retval = set_proc_descriptor_to_pty(&descriptors[ndesc], pty_master_fd, pty_slave_fd);
 	} else {
 		php_error_docref(NULL, E_WARNING, "%s is not a valid descriptor spec/mode", ZSTR_VAL(ztype));
 		goto finish;
@@ -867,14 +866,10 @@ PHP_FUNCTION(proc_open)
 #else
 	char **argv = NULL;
 #endif
+	int pty_master_fd = -1, pty_slave_fd = -1;
 	php_process_id_t child;
 	struct php_process_handle *proc;
 
-#if PHP_CAN_DO_PTS
-	php_file_descriptor_t dev_ptmx = -1;	/* master */
-	php_file_descriptor_t slave_pty = -1;
-#endif
-
 	ZEND_PARSE_PARAMETERS_START(3, 6)
 		Z_PARAM_ZVAL(command_zv)
 		Z_PARAM_ARRAY(descriptorspec)
@@ -957,7 +952,8 @@ PHP_FUNCTION(proc_open)
 				goto exit_fail;
 			}
 		} else if (Z_TYPE_P(descitem) == IS_ARRAY) {
-			if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex) == FAILURE) {
+			if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex,
+				                                 &pty_master_fd, &pty_slave_fd) == FAILURE) {
 				goto exit_fail;
 			}
 		} else {
@@ -1044,27 +1040,6 @@ PHP_FUNCTION(proc_open)
 	if (child == 0) {
 		/* this is the child process */
 
-#if PHP_CAN_DO_PTS
-		if (dev_ptmx >= 0) {
-			int my_pid = getpid();
-
-#ifdef TIOCNOTTY
-			/* detach from original tty. Might only need this if isatty(0) is true */
-			ioctl(0,TIOCNOTTY,NULL);
-#else
-			setsid();
-#endif
-			/* become process group leader */
-			setpgid(my_pid, my_pid);
-			tcsetpgrp(0, my_pid);
-		}
-
-		if (dev_ptmx >= 0) {
-			close(dev_ptmx);
-			close(slave_pty);
-		}
-#endif
-
 		if (close_parent_ends_of_pipes_in_child(descriptors, ndesc) == FAILURE) {
 			/* We are already in child process and can't do anything to make
 			 *   proc_open() return an error in the parent
@@ -1121,13 +1096,6 @@ PHP_FUNCTION(proc_open)
 #endif
 	proc->env = env;
 
-#if PHP_CAN_DO_PTS
-	if (dev_ptmx >= 0) {
-		close(dev_ptmx);
-		close(slave_pty);
-	}
-#endif
-
 	/* clean up all the child ends and then open streams on the parent
 	 * ends, where appropriate */
 	for (i = 0; i < ndesc; i++) {
@@ -1191,7 +1159,14 @@ PHP_FUNCTION(proc_open)
 #else
 	efree_argv(argv);
 #endif
-
+#if HAVE_OPENPTY
+	if (pty_master_fd != -1) {
+		close(pty_master_fd);
+	}
+	if (pty_slave_fd != -1) {
+		close(pty_slave_fd);
+	}
+#endif
 	efree(descriptors);
 	ZVAL_RES(return_value, zend_register_resource(proc, le_proc_open));
 	return;
@@ -1211,12 +1186,12 @@ PHP_FUNCTION(proc_open)
 #else
 	efree_argv(argv);
 #endif
-#if PHP_CAN_DO_PTS
-	if (dev_ptmx >= 0) {
-		close(dev_ptmx);
+#if HAVE_OPENPTY
+	if (pty_master_fd != -1) {
+		close(pty_master_fd);
 	}
-	if (slave_pty >= 0) {
-		close(slave_pty);
+	if (pty_slave_fd != -1) {
+		close(pty_slave_fd);
 	}
 #endif
 	RETURN_FALSE;
diff --git a/ext/standard/tests/file/bug69442.phpt b/ext/standard/tests/file/bug69442.phpt
@@ -17,7 +17,7 @@ EOC;
     $output = join("\n", $output);
     unlink($tmpFile);
 
-    if (strstr($output, "PTY pseudo terminal not supported on this system") !== false) {
+    if (strstr($output, "PTY (pseudoterminal) not supported on this system") !== false) {
         die("skip PTY pseudo terminals are not supported");
     }
 --FILE--
@@ -28,17 +28,31 @@ $pipes = array();
 
 $process = proc_open($cmd, $descriptors, $pipes);
 
-foreach ($pipes as $type => $pipe) {
-    $data = fread($pipe, 999);
-    echo 'type ' . $type . ' ';
-    var_dump($data);
-    fclose($pipe);
+function read_from_pipe($pipe) {
+    $result = fread($pipe, 1000);
+    /* We can't guarantee that everything written to the pipe will be returned by a single call
+     *   to fread(), even if it was written with a single syscall and the number of bytes written
+     *   was small */
+    $again  = @fread($pipe, 1000);
+    if ($again) {
+        $result .= $again;
+    }
+    return $result;
 }
+
+$data0 = read_from_pipe($pipes[0]);
+echo 'read from pipe 0: ';
+var_dump($data0);
+fclose($pipes[0]);
+
+$data3 = read_from_pipe($pipes[3]);
+echo 'read from pipe 3: ';
+var_dump($data3);
+fclose($pipes[3]);
+
 proc_close($process);
 --EXPECT--
-type 0 string(5) "foo
+read from pipe 0: string(5) "foo
 "
-type 1 string(0) ""
-type 2 string(0) ""
-type 3 string(3) "42
+read from pipe 3: string(3) "42
 "
