Skip to content

ext/standard: Refactor unserialization callback function call - #17484

Closed
Girgias wants to merge 2 commits into
php:masterfrom
Girgias:unserialize-callback-refactor
Closed

ext/standard: Refactor unserialization callback function call#17484
Girgias wants to merge 2 commits into
php:masterfrom
Girgias:unserialize-callback-refactor

Conversation

@Girgias

@Girgias Girgias commented Jan 16, 2025

Copy link
Copy Markdown
Member

This also stores the callback function name as a zend_string

Comment on lines +1250 to +1251
/* Find unserialize callback */
zend_function *callback_fn = zend_hash_find_ptr_lc(EG(function_table), PG(unserialize_callback_func));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks some valid callback names, such as:

  • \foo (leading \)
  • Foo::bar (static method)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn't think of static methods, I'll add some tests to ensure this doesn't get missed. And I'll think about a solution if there is a good one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand why you replaced call_user_function() with a manual function lookup + zend_call_known_function. Is there a particular reason?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to see if it is possible to get rid of the function_name field of the FCI struct, and there are not many uses of call_user_function() (currently 29) on top of it having the now useless symbol_table argument (yes I know it's a macro so it doesn't matter).

Comment on lines +1252 to +1256
if (callback_fn == NULL) {
zend_string_release_ex(class_name, 0);
zend_throw_error(NULL, "Unserialization function %s is not defined", ZSTR_VAL(PG(unserialize_callback_func)));
return 0;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This uses space indentation

@Girgias

Girgias commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

Going to close this, as this will need a better approach and part of the content of this PR was split into #22814 and #22933

@Girgias Girgias closed this Jul 30, 2026
@Girgias
Girgias deleted the unserialize-callback-refactor branch July 30, 2026 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants