bug #45851 [Console] Fix exit status on uncaught exception with negative code (acoulton)

fabpot · fabpot · commit 7b43f0f8ef51 · 2022-03-27T09:46:08.000+02:00
This PR was merged into the 4.4 branch. Discussion ---------- [Console] Fix exit status on uncaught exception with negative code | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #45850 | License | MIT | Doc PR | N/A As described in #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 PR 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. IMO therefore applications using negative statuses are relying on an undocumented / incorrect implementation, and this is not a formal BC break. 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. Commits ------- e7072aa [Console] Fix exit status on uncaught exception with negative code
diff --git a/src/Symfony/Component/Console/Application.php b/src/Symfony/Component/Console/Application.php
@@ -157,7 +157,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
@@ -1175,6 +1175,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
`@@ -157,7 +157,7 @@ public function run(InputInterface $input = null, OutputInterface $output = null`
`157`	`157`	`$exitCode = $e->getCode();`
`158`	`158`	`if (is_numeric($exitCode)) {`
`159`	`159`	`$exitCode = (int) $exitCode;`
`160`		`- if (0 === $exitCode) {`
	`160`	`+ if ($exitCode <= 0) {`
`161`	`161`	`$exitCode = 1;`
`162`	`162`	`}`
`163`	`163`	`} else {`