Catch exceptions without capturing them to variables #5345

MaxSem · 2020-04-03T21:26:31Z

See the RFC currently under discussion: https://wiki.php.net/rfc/non-capturing_catches

nikic · 2020-04-06T08:27:48Z

You will also need to adjust the ZEND_CATCH implementation with a RETURN_VALUE_USED check. Right now, you're going to clobber some random piece of stack memory with the exception.

MaxSem · 2020-04-06T08:42:33Z

Yeah, I noticed it was leaking, just haven't figured out where it should go yet.

nikic · 2020-04-06T08:43:51Z

Adding a check around here should do it:

php-src/Zend/zend_vm_def.h

Line 4489 in 21cfa03

ex = EX_VAR(opline->result.var);

If RETURN_VALUE_USED do what the code does now, otherwise OBJ_RELEASE(EG(exception)).

TysonAndre · 2020-04-11T19:54:22Z

👍 for this, I'd rather avoid the unnecessary variable for catch (Exception $unused) or catch (Exception $_)

It'd be useful to add a test of what happens when the destructor gets called, and to fix issues with destructors (keep the test in case this is improperly handled by opcache or the JIT in the future)

I'd imagine the handling of __destruct should be like catch(Exception $e) { unset($e); ... } but be more efficient.

I think it does the right thing right now, because the catch opcode already needs to account for what happens if $e was already an object that could throw in __destruct().

<?php

class ThrowsOnDestruct extends Exception {
    public function __destruct() {
        echo "Throwing\n";
        throw new RuntimeException(__METHOD__);
    }
}
try {
    throw new ThrowsOnDestruct();
} catch (Exception) {
    echo "Unreachable catch\n";
}
echo "Unreachable fallthrough\n";

When I try to run this example (on a branch including this PR and the throws exception PR), I see an unexpected Fatal error: Attempt to destruct pending exception

» php test.php 

Warning: Uncaught ThrowsOnDestruct in /path/to/test.php:9
Stack trace:
#0 {main}
  thrown in /path/to/test.php on line 9

Fatal error: Attempt to destruct pending exception in Unknown on line 0

Majkl578 · 2020-04-13T19:08:57Z

I actually started a patch for this 4 years ago but never forced myself to write an RFC. 😄 So 💯+🥇!

nikic · 2020-05-11T09:19:44Z

Implementation looks fine to me. Don't think any opcache changes needed either, as we bail out on try/catch for all the tricky parts.

Zend/tests/try/catch_novar.phpt

MaxSem · 2020-05-13T09:41:24Z

@TysonAndre, yep - committed a test for it, investigating how to fix it.

iluuu1994 · 2020-05-25T21:55:57Z

Zend/tests/try/catch_novar_2.phpt

+}
+try {
+    throw new ThrowsOnDestruct();
+} catch (Exception $ex) {


Shouldn't $ex be removed here?

I must've reinstated it during my experiments. Fixed.

TysonAndre

A suggested fix for the test exiting without properly reporting the uncaught exception - @nikic might know more about the edge case I'm worried about.

I'm also not sure why the GC_ADDREF leads to a memory leak when the exception has no variable, but taking it out seems to work.

The test for catch_novar_2.phpt seems to be crashing. I think that EG(exception) should be set to null before calling the destructor on the caught exception.

I'm not 100% sure of what to do about public function __destruct() { throw $this; }. The below case is different in that the RuntimeException is the uncaught exception which still has a reference count, not the ThrowsOnDestruct. Maybe the object still exists but __destruct won't be called again?

php > class ThrowsOnDestruct extends Exception { public function __destruct() { echo "throwing self\n"; throw $this; }}
php > $e = new ThrowsOnDestruct();
php > try { throw new RuntimeException(); } catch (Exception $e) {}
throwing self
Warning: Uncaught RuntimeException in php shell code:1
Stack trace:
#0 {main}

Next ThrowsOnDestruct in php shell code:1
Stack trace:
#0 {main}
  thrown in php shell code on line 1

diff --git a/Zend/tests/try/catch_novar_2.phpt b/Zend/tests/try/catch_novar_2.phpt
index 60a7ebb283..846fb86484 100644
--- a/Zend/tests/try/catch_novar_2.phpt
+++ b/Zend/tests/try/catch_novar_2.phpt
@@ -15,5 +15,11 @@ try {
 }
 echo "Unreachable fallthrough\n";
 
---EXPECT--
+--EXPECTF--
 Throwing
+
+Fatal error: Uncaught RuntimeException: ThrowsOnDestruct::__destruct in %scatch_novar_2.php:5
+Stack trace:
+#0 %scatch_novar_2.php(10): ThrowsOnDestruct->__destruct()
+#1 {main}
+  thrown in %scatch_novar_2.php on line 5
diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index ab44b9ca7c..20058b5337 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -4493,15 +4493,22 @@ ZEND_VM_HANDLER(107, ZEND_CATCH, CONST, JMP_ADDR, LAST_CATCH|CACHE_SLOT)
 		}
 		zval_ptr_dtor(ex);
 		ZVAL_OBJ(ex, EG(exception));
-	} else {
-		OBJ_RELEASE(EG(exception));
-	}
-	if (UNEXPECTED(EG(exception) != exception)) {
-		GC_ADDREF(EG(exception));
-		HANDLE_EXCEPTION();
+		if (UNEXPECTED(EG(exception) != exception)) {
+			GC_ADDREF(EG(exception));
+			HANDLE_EXCEPTION();
+		} else {
+			EG(exception) = NULL;
+			ZEND_VM_NEXT_OPCODE();
+		}
 	} else {
 		EG(exception) = NULL;
-		ZEND_VM_NEXT_OPCODE();
+		OBJ_RELEASE(exception);
+		// TODO: What about function __destruct() { throw $this; }
+		if (UNEXPECTED(EG(exception) != NULL)) {
+			HANDLE_EXCEPTION();
+		} else {
+			ZEND_VM_NEXT_OPCODE();
+		}
 	}
 }
 
diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h
index 7ecc72a280..b8f511075a 100644
--- a/Zend/zend_vm_execute.h
+++ b/Zend/zend_vm_execute.h
@@ -3709,15 +3709,21 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_CATCH_SPEC_CONST_HANDLER(ZEND_
 		}
 		zval_ptr_dtor(ex);
 		ZVAL_OBJ(ex, EG(exception));
-	} else {
-		OBJ_RELEASE(EG(exception));
-	}
-	if (UNEXPECTED(EG(exception) != exception)) {
-		GC_ADDREF(EG(exception));
-		HANDLE_EXCEPTION();
+		if (UNEXPECTED(EG(exception) != exception)) {
+			GC_ADDREF(EG(exception));
+			HANDLE_EXCEPTION();
+		} else {
+			EG(exception) = NULL;
+			ZEND_VM_NEXT_OPCODE();
+		}
 	} else {
 		EG(exception) = NULL;
-		ZEND_VM_NEXT_OPCODE();
+		OBJ_RELEASE(exception);
+		if (UNEXPECTED(EG(exception) != NULL)) {
+			HANDLE_EXCEPTION();
+		} else {
+			ZEND_VM_NEXT_OPCODE();
+		}
 	}
 }

TysonAndre · 2020-05-25T22:50:19Z

Zend/tests/try/catch_novar_2.phpt

+echo "Unreachable fallthrough\n";
+
+--EXPECT--
+Throwing


This looks as if the php process exited - normally, if it rethrew, php's output would include a message about an uncaught exception and a stack trace.

The RFC is currently being drafted at https://wiki.php.net/rfc/non-capturing_catches

nikic · 2020-05-26T09:43:18Z

I've pushed a fix for the throw in dtor issue, basically following @TysonAndre's suggestion above.

@TysonAndre 81c824f should make it more obvious why the GC_ADDREF doesn't make sense in this case.

I'm not 100% sure of what to do about public function __destruct() { throw $this; }. The below case is different in that the RuntimeException is the uncaught exception which still has a reference count, not the ThrowsOnDestruct. Maybe the object still exists but __destruct won't be called again?

That's correct.

nikic · 2020-05-26T09:45:28Z

Zend/zend_vm_def.h

+		zval *ex = EX_VAR(opline->result.var);
+		if (UNEXPECTED(Z_ISREF_P(ex))) {
+			ex = Z_REFVAL_P(ex);
+		}


Hm... not related to your patch, but I just realized that this can bypass typed references.

This turned into something of a rabbit hole, but is now addressed with 314ab47 and 4a08ca1.

…ch` sniff > Allow catching exceptions without capturing them to variables Refs: * https://wiki.php.net/rfc/non-capturing_catches * php/php-src#5345 * php/php-src@23ee4d4 Includes unit tests.

nikic added the RFC label Apr 3, 2020

MaxSem force-pushed the catch branch from 1e412f9 to a8ffd76 Compare April 4, 2020 17:22

MaxSem force-pushed the catch branch from a8ffd76 to d78f68f Compare April 6, 2020 22:54

rybakit reviewed May 11, 2020

View reviewed changes

Zend/tests/try/catch_novar.phpt Outdated Show resolved Hide resolved

MaxSem force-pushed the catch branch from d78f68f to d44af03 Compare May 13, 2020 09:39

iluuu1994 reviewed May 25, 2020

View reviewed changes

MaxSem force-pushed the catch branch from d44af03 to 8663565 Compare May 25, 2020 22:29

TysonAndre requested changes May 25, 2020

View reviewed changes

MaxSem and others added 2 commits May 26, 2020 11:35

[RFC] Catch exceptions without capturing them to variables

78c599b

The RFC is currently being drafted at https://wiki.php.net/rfc/non-capturing_catches

Fix throw during exception destruction

b77cf48

nikic force-pushed the catch branch from 8663565 to b77cf48 Compare May 26, 2020 09:39

nikic reviewed May 26, 2020

View reviewed changes

php-pulls closed this in 23ee4d4 May 26, 2020

MaxSem deleted the catch branch May 26, 2020 13:39

carusogabriel added this to the PHP 8.0 milestone May 29, 2020

TomasVotruba mentioned this pull request Jun 3, 2020

[PHP 8.0] Add exception without variable nikic/PHP-Parser#666

Merged

jrfnl mentioned this pull request Jun 29, 2020

PHP 8.0: New PHPCompatibility.ControlStructures.NewNonCapturingCatch sniff PHPCompatibility/PHPCompatibility#1151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch exceptions without capturing them to variables #5345

Catch exceptions without capturing them to variables #5345

MaxSem commented Apr 3, 2020 •

edited

nikic commented Apr 6, 2020

MaxSem commented Apr 6, 2020

nikic commented Apr 6, 2020

TysonAndre commented Apr 11, 2020 •

edited

Majkl578 commented Apr 13, 2020 •

edited

nikic commented May 11, 2020

MaxSem commented May 13, 2020

iluuu1994 May 25, 2020

MaxSem May 25, 2020

TysonAndre left a comment •

edited

TysonAndre May 25, 2020

nikic commented May 26, 2020

nikic May 26, 2020

nikic May 26, 2020

Catch exceptions without capturing them to variables #5345

Catch exceptions without capturing them to variables #5345

Conversation

MaxSem commented Apr 3, 2020 • edited

nikic commented Apr 6, 2020

MaxSem commented Apr 6, 2020

nikic commented Apr 6, 2020

TysonAndre commented Apr 11, 2020 • edited

Majkl578 commented Apr 13, 2020 • edited

nikic commented May 11, 2020

MaxSem commented May 13, 2020

iluuu1994 May 25, 2020

Choose a reason for hiding this comment

MaxSem May 25, 2020

Choose a reason for hiding this comment

TysonAndre left a comment • edited

Choose a reason for hiding this comment

TysonAndre May 25, 2020

Choose a reason for hiding this comment

nikic commented May 26, 2020

nikic May 26, 2020

Choose a reason for hiding this comment

nikic May 26, 2020

Choose a reason for hiding this comment

MaxSem commented Apr 3, 2020 •

edited

TysonAndre commented Apr 11, 2020 •

edited

Majkl578 commented Apr 13, 2020 •

edited

TysonAndre left a comment •

edited