ConsoleReader: Use proc_open()'s socket support to send commands back to the main server process (#5273)

Support for this was introduced in PHP 8.0, though not mentioned in any changelog: php/php-src#5777

This simplifies the subprocess handling considerably. However, there is a potential for problems if PHP generates any E_* errors, since these get written to STDOUT as well.
To avoid error messages being treated as a command, a hash is attached to each IPC message, seeded with an incrementing counter. This prevents error messages causing command replays or unintended commands.

Unfortunately, PHP doesn't support binding pipes other than stdin/stdout/stderr on Windows for the child process, so we have to use stdout for this. In the future, if it becomes possible, a dedicated pipe for the purpose should be introduced. We'd need something like php://fd/<number> to work on Windows.

Author: dktapps

src/console/ConsoleReaderChildProcess.php

@@ -29,23 +29,21 @@
 use function cli_set_process_title;
 use function count;
 use function dirname;
-use function feof;
 use function fwrite;
-use function stream_socket_client;
+use function is_numeric;
+use const PHP_EOL;
+use const STDOUT;
 
-require dirname(__DIR__, 2) . '/vendor/autoload.php';
-
-if(count($argv) !== 2){
-	die("Please provide a server to connect to");
+if(count($argv) !== 2 || !is_numeric($argv[1])){
+	echo "Usage: " . $argv[0] . " <command token seed>" . PHP_EOL;
+	exit(1);
 }
 
+$commandTokenSeed = (int) $argv[1];
+
+require dirname(__DIR__, 2) . '/vendor/autoload.php';
+
 @cli_set_process_title('PocketMine-MP Console Reader');
-$errCode = null;
-$errMessage = null;
-$socket = stream_socket_client($argv[1], $errCode, $errMessage, 15.0);
-if($socket === false){
-	throw new \RuntimeException("Failed to connect to server process ($errCode): $errMessage");
-}
 
 /** @phpstan-var ThreadSafeArray<int, string> $channel */
 $channel = new ThreadSafeArray();
@@ -75,15 +73,15 @@ public function run() : void{
 };
 
 $thread->start(NativeThread::INHERIT_NONE);
-while(!feof($socket)){
+while(true){
 	$line = $channel->synchronized(function() use ($channel) : ?string{
 		if(count($channel) === 0){
 			$channel->wait(1_000_000);
 		}
-		$line = $channel->shift();
-		return $line;
+		return $channel->shift();
 	});
-	if(@fwrite($socket, ($line ?? "") . "\n") === false){
+	$message = $line !== null ? ConsoleReaderChildProcessUtils::createMessage($line, $commandTokenSeed) : "";
+	if(@fwrite(STDOUT, $message . "\n") === false){
 		//Always send even if there's no line, to check if the parent is alive
 		//If the parent process was terminated forcibly, it won't close the connection properly, so feof() will return
 		//false even though the connection is actually broken. However, fwrite() will fail.

src/console/ConsoleReaderChildProcessDaemon.php

@@ -29,19 +29,16 @@
 use function base64_encode;
 use function fgets;
 use function fopen;
+use function mt_rand;
 use function preg_replace;
 use function proc_close;
 use function proc_open;
 use function proc_terminate;
+use function rtrim;
 use function sprintf;
 use function stream_select;
-use function stream_socket_accept;
-use function stream_socket_get_name;
-use function stream_socket_server;
-use function stream_socket_shutdown;
 use function trim;
 use const PHP_BINARY;
-use const STREAM_SHUT_RDWR;
 
 /**
  * This pile of shit exists because PHP on Windows is broken, and can't handle stream_select() on stdin or pipes
@@ -58,44 +55,44 @@
  * communication.
  */
 final class ConsoleReaderChildProcessDaemon{
+	public const TOKEN_DELIMITER = ":";
+	public const TOKEN_HASH_ALGO = "xxh3";
+
 	private \PrefixedLogger $logger;
 	/** @var resource */
 	private $subprocess;
 	/** @var resource */
 	private $socket;
+	private int $commandTokenSeed;
 
 	public function __construct(
 		\Logger $logger
 	){
 		$this->logger = new \PrefixedLogger($logger, "Console Reader Daemon");
+		$this->commandTokenSeed = mt_rand();
 		$this->prepareSubprocess();
 	}
 
 	private function prepareSubprocess() : void{
-		$server = stream_socket_server("tcp://127.0.0.1:0");
-		if($server === false){
-			throw new \RuntimeException("Failed to open console reader socket server");
-		}
-		$address = Utils::assumeNotFalse(stream_socket_get_name($server, false), "stream_socket_get_name() shouldn't return false here");
-
 		//Windows sucks, and likes to corrupt UTF-8 file paths when they travel to the subprocess, so we base64 encode
 		//the path to avoid the problem. This is an abysmally shitty hack, but here we are :(
 		$sub = Utils::assumeNotFalse(proc_open(
-			[PHP_BINARY, '-dopcache.enable_cli=0', '-r', sprintf('require base64_decode("%s", true);', base64_encode(Path::join(__DIR__, 'ConsoleReaderChildProcess.php'))), $address],
 			[
+				PHP_BINARY,
+				'-dopcache.enable_cli=0',
+				'-r',
+				sprintf('require base64_decode("%s", true);', base64_encode(Path::join(__DIR__, 'ConsoleReaderChildProcess.php'))),
+				(string) $this->commandTokenSeed
+			],
+			[
+				1 => ['socket'],
 				2 => fopen("php://stderr", "w"),
 			],
 			$pipes
 		), "Something has gone horribly wrong");
 
-		$client = stream_socket_accept($server, 15);
-		if($client === false){
-			throw new AssumptionFailedError("stream_socket_accept() returned false");
-		}
-		stream_socket_shutdown($server, STREAM_SHUT_RDWR);
-
 		$this->subprocess = $sub;
-		$this->socket = $client;
+		$this->socket = $pipes[1];
 	}
 
 	private function shutdownSubprocess() : void{
@@ -104,21 +101,34 @@ private function shutdownSubprocess() : void{
 		//the first place).
 		proc_terminate($this->subprocess);
 		proc_close($this->subprocess);
-		stream_socket_shutdown($this->socket, STREAM_SHUT_RDWR);
 	}
 
 	public function readLine() : ?string{
 		$r = [$this->socket];
 		$w = null;
 		$e = null;
 		if(stream_select($r, $w, $e, 0, 0) === 1){
-			$command = fgets($this->socket);
-			if($command === false){
+			$line = fgets($this->socket);
+			if($line === false){
 				$this->logger->debug("Lost connection to subprocess, restarting (maybe the child process was killed from outside?)");
 				$this->shutdownSubprocess();
 				$this->prepareSubprocess();
 				return null;
 			}
+			$line = rtrim($line, "\n");
+
+			if($line === ""){
+				//keepalive
+				return null;
+			}
+
+			$command = ConsoleReaderChildProcessUtils::parseMessage($line, $this->commandTokenSeed);
+			if($command === null){
+				//this is not a command - it may be some kind of error output from the subprocess
+				//write it directly to the console
+				$this->logger->warning("Unexpected output from child process: $line");
+				return null;
+			}
 
 			$command = preg_replace("#\\x1b\\x5b([^\\x1b]*\\x7e|[\\x40-\\x50])#", "", trim($command)) ?? throw new AssumptionFailedError("This regex is assumed to be valid");
 			$command = preg_replace('/[[:cntrl:]]/', '', $command) ?? throw new AssumptionFailedError("This regex is assumed to be valid");

src/console/ConsoleReaderChildProcessUtils.php

@@ -0,0 +1,71 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+ */
+
+declare(strict_types=1);
+
+namespace pocketmine\console;
+
+use function hash;
+use function strlen;
+use function strrpos;
+use function substr;
+
+final class ConsoleReaderChildProcessUtils{
+	public const TOKEN_DELIMITER = ":";
+	public const TOKEN_HASH_ALGO = "xxh3";
+
+	private function __construct(){
+
+	}
+
+	/**
+	 * Creates an IPC message to transmit a user's input command to the parent process.
+	 *
+	 * Unfortunately we can't currently provide IPC pipes other than stdout/stderr to subprocesses on Windows, so this
+	 * adds a hash of the user input (with a counter as salt) to prevent unintended process output (like error messages)
+	 * from being treated as user input.
+	 */
+	public static function createMessage(string $line, int &$counter) : string{
+		$token = hash(self::TOKEN_HASH_ALGO, $line, options: ['seed' => $counter]);
+		$counter++;
+		return $line . self::TOKEN_DELIMITER . $token;
+	}
+
+	/**
+	 * Extracts a command from an IPC message from the console reader subprocess.
+	 * Returns the user's input command, or null if this isn't a user input.
+	 */
+	public static function parseMessage(string $message, int &$counter) : ?string{
+		$delimiterPos = strrpos($message, self::TOKEN_DELIMITER);
+		if($delimiterPos !== false){
+			$left = substr($message, 0, $delimiterPos);
+			$right = substr($message, $delimiterPos + strlen(self::TOKEN_DELIMITER));
+			$expectedToken = hash(self::TOKEN_HASH_ALGO, $left, options: ['seed' => $counter]);
+
+			if($expectedToken === $right){
+				$counter++;
+				return $left;
+			}
+		}
+
+		return null;
+	}
+}

tests/phpunit/console/ConsoleReaderChildProcessUtilsTest.php

@@ -0,0 +1,92 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+ */
+
+declare(strict_types=1);
+
+namespace pocketmine\console;
+
+use PHPUnit\Framework\TestCase;
+use function mt_rand;
+use function str_repeat;
+
+final class ConsoleReaderChildProcessUtilsTest extends TestCase{
+
+	/**
+	 * @phpstan-return \Generator<int, array{string}, void, void>
+	 */
+	public static function commandStringProvider() : \Generator{
+		yield ["stop"];
+		yield ["pocketmine:status"];
+		yield [str_repeat("s", 1000)];
+		yield ["time set day"];
+		yield ["give \"Steve\" golden_apple"];
+	}
+
+	/**
+	 * @dataProvider commandStringProvider
+	 */
+	public function testCreateParseSymmetry(string $input) : void{
+		$counterCreate = $counterParse = mt_rand();
+		$message = ConsoleReaderChildProcessUtils::createMessage($input, $counterCreate);
+		$parsedInput = ConsoleReaderChildProcessUtils::parseMessage($message, $counterParse);
+
+		self::assertSame($input, $parsedInput);
+	}
+
+	public function testCreateMessage() : void{
+		$counter = 0;
+
+		ConsoleReaderChildProcessUtils::createMessage("", $counter);
+		self::assertSame(1, $counter, "createMessage should always bump the counter");
+	}
+
+	/**
+	 * @phpstan-return \Generator<int, array{string, bool}, void, void>
+	 */
+	public static function parseMessageProvider() : \Generator{
+		$counter = 0;
+		yield [ConsoleReaderChildProcessUtils::createMessage("", $counter), true];
+
+		yield ["", false]; //keepalive message, doesn't bump counter
+
+		$counter = 1;
+		yield [ConsoleReaderChildProcessUtils::createMessage("", $counter), false]; //mismatched counter
+
+		$counter = 0;
+		yield ["a" . ConsoleReaderChildProcessUtils::TOKEN_DELIMITER . "b", false]; //message with delimiter but not a valid IPC message
+	}
+
+	/**
+	 * @dataProvider parseMessageProvider
+	 */
+	public static function testParseMessage(string $message, bool $valid) : void{
+		$counter = $oldCounter = 0;
+
+		$input = ConsoleReaderChildProcessUtils::parseMessage($message, $counter);
+		if(!$valid){
+			self::assertNull($input, "Result should be null on invalid message");
+			self::assertSame($oldCounter, $counter, "Counter shouldn't be bumped on invalid message");
+		}else{
+			self::assertNotNull($input, "This was a valid message, expected a result");
+			self::assertSame($oldCounter + 1, $counter, "Counter should be bumped on valid message parse");
+		}
+	}
+}