Merge pull request #230 from MarcHagen/patch-1

Fail if `unserialize` is unable to parse output of child process
spatie · Jan 31, 2025 · e1ca936 · e1ca936
2 parents 0fa90e9 + bc87ce6
commit e1ca936
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 11 deletions.
diff --git a/phpunit.xml.dist b/phpunit.xml.dist
@@ -12,6 +12,7 @@
     convertWarningsToExceptions="true"
     processIsolation="false"
     stopOnFailure="false"
+    beStrictAboutTestsThatDoNotTestAnything="true"
 >
     <coverage>
         <include>

diff --git a/src/Process/ParallelProcess.php b/src/Process/ParallelProcess.php
@@ -68,11 +68,15 @@ public function getOutput()
         if (! $this->output) {
             $processOutput = $this->process->getOutput();
 
-            $this->output = @unserialize(base64_decode($processOutput));
+			$childResult = @unserialize(base64_decode($processOutput));
 
-            if (! $this->output) {
+            if ($childResult === false || ! array_key_exists('output', $childResult)) {
                 $this->errorOutput = $processOutput;
+
+				return null;
             }
+
+			$this->output = $childResult['output'];
         }
 
         return $this->output;
@@ -83,11 +87,13 @@ public function getErrorOutput()
         if (! $this->errorOutput) {
             $processOutput = $this->process->getErrorOutput();
 
-            $this->errorOutput = @unserialize(base64_decode($processOutput));
+			$childResult = @unserialize(base64_decode($processOutput));
 
-            if (! $this->errorOutput) {
+            if ($childResult === false || ! array_key_exists('output', $childResult)) {
                 $this->errorOutput = $processOutput;
-            }
+			} else {
+				$this->errorOutput = $childResult['output'];
+			}
         }
 
         return $this->errorOutput;

diff --git a/src/Process/ProcessCallbacks.php b/src/Process/ProcessCallbacks.php
@@ -34,14 +34,14 @@ public function timeout(callable $callback): self
 
     public function triggerSuccess()
     {
+        $output = $this->getOutput();
+
         if ($this->getErrorOutput()) {
             $this->triggerError();
 
             return;
         }
 
-        $output = $this->getOutput();
-
         foreach ($this->successCallbacks as $callback) {
             call_user_func_array($callback, [$output]);
         }

diff --git a/src/Runtime/ChildRuntime.php b/src/Runtime/ChildRuntime.php
@@ -2,6 +2,16 @@
 
 use Spatie\Async\Runtime\ParentRuntime;
 
+// php://stdout does not obey output buffering. Any output would break
+// unserialization of child process results in the parent process.
+if (!defined('STDOUT')) {
+    define('STDOUT', fopen('php://temp', 'w+b'));
+    define('STDERR', fopen('php://stderr', 'wb'));
+}
+
+ini_set('display_startup_errors', 1);
+ini_set('display_errors', 'stderr');
+
 try {
     $autoloader = $argv[1] ?? null;
     $serializedClosure = $argv[2] ?? null;
@@ -23,9 +33,11 @@
 
     $task = ParentRuntime::decodeTask($serializedClosure);
 
+    ob_start();
     $output = call_user_func($task);
+    ob_end_clean();
 
-    $serializedOutput = base64_encode(serialize($output));
+    $serializedOutput = base64_encode(serialize(['output' => $output]));
 
     if (strlen($serializedOutput) > $outputLength) {
         throw \Spatie\Async\Output\ParallelError::outputTooLarge($outputLength);
@@ -39,7 +51,7 @@
 
     $output = new \Spatie\Async\Output\SerializableException($exception);
 
-    fwrite(STDERR, base64_encode(serialize($output)));
+    fwrite(STDERR, base64_encode(serialize(['output' => $output])));
 
     exit(1);
 }
diff --git a/tests/ChildRuntimeTest.php b/tests/ChildRuntimeTest.php
@@ -15,7 +15,8 @@ public function it_can_run()
         $autoloader = __DIR__.'/../vendor/autoload.php';
 
         $serializedClosure = base64_encode(serialize(new SerializableClosure(function () {
-            echo 'child';
+            echo 'interfere with output';
+            return 'child';
         })));
 
         $process = new Process([
@@ -28,7 +29,10 @@ public function it_can_run()
         $process->start();
 
         $process->wait();
+        $output = unserialize(base64_decode($process->getOutput()));
 
-        $this->assertStringContainsString('child', $process->getOutput());
+        $this->assertIsArray($output);
+        $this->assertArrayHasKey('output', $output);
+        $this->assertStringContainsString('child', $output['output']);
     }
 }
diff --git a/tests/ErrorHandlingTest.php b/tests/ErrorHandlingTest.php
@@ -167,6 +167,22 @@ public function it_handles_stderr_as_parallel_error()
         $pool->wait();
     }
 
+    /** @test */
+    public function it_handles_stdout_as_parallel_error()
+    {
+        $pool = Pool::create();
+
+        $pool->add(function () {
+            fwrite(STDOUT, 'test');
+        })->then(function ($output) {
+            $this->fail('Child process output did not error on faulty output');
+        })->catch(function (ParallelError $error) {
+            $this->assertStringContainsString('test', $error->getMessage());
+        });
+
+        $pool->wait();
+    }
+
     /** @test */
     public function deep_syntax_errors_are_thrown()
     {