[Console] Fix exit status on uncaught exception with negative code

acoulton · SanderHagen · commit 9942ff3ff095 · 2022-04-01T14:58:35.000+02:00
As described in symfony#45850, if an application threw an exception with the `code` property set to a negative number this could in some cases cause the process to appear to exit 'successfully' with a zero exit status. Exiting with any negative number produces potentially unexpected results - the reported exit status will not appear to match the value that was set. This is due to the binary handling / truncation of exit codes. This may theoretically break BC for applications that were intentionally throwing exceptions with negative codes and performing some action based on that status. However, given they would have had to implement an algorithm to map e.g. `-10` in PHP to `246` as the actual exit status, it seems unlikely this is a common usage. It is certainly outside the defined behaviour of POSIX exit codes. Therefore I believe it is essentially safe to assume that exceptions with negative codes are e.g. being thrown by lower-level components, and are not intended to set a shell exit status. Coalescing all negative numbers to 1 matches the existing behaviour with other 'invalid' exception codes e.g. empty / zero / non-numeric. This therefore feels the most robust fix and eliminates any potential for confusion. Fixes symfony#45850
diff --git a/src/Symfony/Component/Console/Application.php b/src/Symfony/Component/Console/Application.php
@@ -179,7 +179,7 @@ public function run(InputInterface $input = null, OutputInterface $output = null
             $exitCode = $e->getCode();
             if (is_numeric($exitCode)) {
                 $exitCode = (int) $exitCode;
-                if (0 === $exitCode) {
+                if ($exitCode <= 0) {
                     $exitCode = 1;
                 }
             } else {
diff --git a/src/Symfony/Component/Console/Tests/ApplicationTest.php b/src/Symfony/Component/Console/Tests/ApplicationTest.php
@@ -1172,6 +1172,25 @@ public function testRunDispatchesExitCodeOneForExceptionCodeZero()
         $this->assertTrue($passedRightValue, '-> exit code 1 was passed in the console.terminate event');
     }
 
+    /**
+     * @testWith [-1]
+     *           [-32000]
+     */
+    public function testRunReturnsExitCodeOneForNegativeExceptionCode($exceptionCode)
+    {
+        $exception = new \Exception('', $exceptionCode);
+
+        $application = $this->getMockBuilder(Application::class)->setMethods(['doRun'])->getMock();
+        $application->setAutoExit(false);
+        $application->expects($this->once())
+            ->method('doRun')
+            ->willThrowException($exception);
+
+        $exitCode = $application->run(new ArrayInput([]), new NullOutput());
+
+        $this->assertSame(1, $exitCode, '->run() returns exit code 1 when exception code is '.$exceptionCode);
+    }
+
     public function testAddingOptionWithDuplicateShortcut()
     {
         $this->expectException(\LogicException::class);

Original file line number	Diff line number	Diff line change
`@@ -179,7 +179,7 @@ public function run(InputInterface $input = null, OutputInterface $output = null`
`179`	`179`	`$exitCode = $e->getCode();`
`180`	`180`	`if (is_numeric($exitCode)) {`
`181`	`181`	`$exitCode = (int) $exitCode;`
`182`		`- if (0 === $exitCode) {`
	`182`	`+ if ($exitCode <= 0) {`
`183`	`183`	`$exitCode = 1;`
`184`	`184`	`}`
`185`	`185`	`} else {`