Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertion failure in Zend/zend_compile.c #15907

Open
YuanchengJiang opened this issue Sep 16, 2024 · 8 comments
Open

Assertion failure in Zend/zend_compile.c #15907

YuanchengJiang opened this issue Sep 16, 2024 · 8 comments

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
set_error_handler(function($_, $msg) {
throw new Exception($msg);
});
class UnSerializable implements Serializable
{
public function serialize() {}
public function unserialize($serialized) {}
}

Resulted in this output:

/php-src/Zend/zend_compile.c:1334: zend_bind_class_in_slot: Assertion `!(executor_globals.exception)' failed.
Aborted (core dumped)

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

@cmb69
Copy link
Member

cmb69 commented Sep 16, 2024

Relevant commit: c8fa477

Maybe the assertion is just too presumptious, given that an error handler may throw. Removing it, makes the given script pass. But that requires more thorough investigation, and I'm somewhat concerned that existing observers might make the same assumption.

Tentatively qualifying this as bug.

@cmb69 cmb69 changed the title Core dumped in Zend/zend_compile.c Assertion failure in Zend/zend_compile.c Sep 16, 2024
@iluuu1994
Copy link
Member

I mean, this is a common programming idiom, so this is definitely a bug. Technically it doesn't seem strictly necessary to bail here, since inheritance wasn't aborted. However, note that inheritance may still accidentally abort due to EG(exception) being set.

After removing the ZEND_ASSERT():

set_error_handler(function($_, $msg) {
    throw new Exception($msg);
});

spl_autoload_register(function() {});

class P {
    public function test(): X {}
}

class UnSerializable extends P implements Serializable {
    public function serialize() {}
    public function unserialize($serialized) {}
    public function test(): Y {}
}

Fatal error: During inheritance of UnSerializable, while autoloading Y: Uncaught Exception: UnSerializable implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary)

Not sure what the best resolution is.

@cmb69
Copy link
Member

cmb69 commented Sep 17, 2024

I mean, this is a common programming idiom, […]

You are referring to throwing from an error handler? Yeah, that would be common.

But imagine there were no error handlers …

@iluuu1994
Copy link
Member

I don't follow. 🙂

@cmb69
Copy link
Member

cmb69 commented Sep 17, 2024

If there were no error handlers, this ticket would be invalid, and more generally section 3.5 of https://biagiocosenza.com/papers/PopovCC17.pdf would no longer apply (and yes, I'm aware that $errcontext is gone as of PHP 8.0.0, but still error handlers allow for spooky action at a distance).

@iluuu1994
Copy link
Member

iluuu1994 commented Sep 17, 2024

Ah ^^ I agree that error handlers were a bad idea. I spent quite a bit of time last year trying to solve related issues. I also looked into alternatives with the possibility of deprecating them, but Nicolas Grekas didn't consider the alternative to be feature-complete.

@iluuu1994
Copy link
Member

iluuu1994 commented Sep 17, 2024

I would suggest adding a if (EG(exception)) zend_exception_uncaught_error(); here:

zend_error(E_DEPRECATED, "%s implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary)", ZSTR_VAL(class_type->name));

Analogous to the code in c8fa477. Given the example from this comment, it isn't currently possible to handle this gracefully, as other code paths can still trigger a fatal error when promoting these warnings to exceptions.

WDYT?

@cmb69
Copy link
Member

cmb69 commented Sep 18, 2024

That could be

 Zend/zend_interfaces.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Zend/zend_interfaces.c b/Zend/zend_interfaces.c
index f3fb4151a5..7fc8a7bcfb 100644
--- a/Zend/zend_interfaces.c
+++ b/Zend/zend_interfaces.c
@@ -478,6 +478,9 @@ static int zend_implement_serializable(zend_class_entry *interface, zend_class_e
 	if (!(class_type->ce_flags & ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)
 			&& (!class_type->__serialize || !class_type->__unserialize)) {
 		zend_error(E_DEPRECATED, "%s implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary)", ZSTR_VAL(class_type->name));
+		if (EG(exception)) {
+			zend_exception_uncaught_error("While implementing the Serializable interface");
+		}
 	}
 	return SUCCESS;
 }

I think this makes perfect sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants