Skip to content

Assertion failure in Zend/zend_compile.c #15907

Closed
Closed
Assertion failure in Zend/zend_compile.c#15907
Labels
BugCategory: EngineStatus: Verified

Description

@YuanchengJiang
YuanchengJiang
opened on Sep 16, 2024

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

Activity

  1. YuanchengJiang
    added
    Bug
    Status: Needs Triage
    on Sep 16, 2024

  2. cmb69 commented on Sep 16, 2024

    @cmb69
    cmb69
    on Sep 16, 2024
    Member

    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.

  3. cmb69
    added
    Category: Engine
    Status: Verified
    and removed
    Status: Needs Triage
    on Sep 16, 2024
  4. cmb69
    changed the title [-]Core dumped in Zend/zend_compile.c[/-] [+]Assertion failure in Zend/zend_compile.c[/+] on Sep 16, 2024

  5. iluuu1994 commented on Sep 17, 2024

    @iluuu1994
    iluuu1994
    on Sep 17, 2024
    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.

  6. cmb69 commented on Sep 17, 2024

    @cmb69
    cmb69
    on Sep 17, 2024
    Member

    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 …

  7. iluuu1994 commented on Sep 17, 2024

    @iluuu1994
    iluuu1994
    on Sep 17, 2024
    Member

    I don't follow. 🙂

  8. cmb69 commented on Sep 17, 2024

    @cmb69
    cmb69
    on Sep 17, 2024
    Member

    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).

  9. iluuu1994 commented on Sep 17, 2024

    @iluuu1994
    iluuu1994
    on Sep 17, 2024
    Member

    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.

  10. iluuu1994 commented on Sep 17, 2024

    @iluuu1994
    iluuu1994
    on Sep 17, 2024
    Member

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

    php-src/Zend/zend_interfaces.c

    Line 480 in 43dc2eb

    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?

  11. cmb69 commented on Sep 18, 2024

    @cmb69
    cmb69
    on Sep 18, 2024
    Member

    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.

  12. iluuu1994
    added a commit that references this issue on Sep 26, 2024

    Fix failed assertion when promoting Serialize deprecation to exception

    15a0c3a
  13. iluuu1994
    closed this as completedin 15a0c3aon Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugCategory: EngineStatus: Verified

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions