Skip to content

Add per-extension child initialization hooks - #24647

Merged
arnaud-lb merged 1 commit into
php:masterfrom
kairosci:feat/gh-22616-extension-child-init
Nov 11, 2026
Merged

arnaud-lb merged 1 commit into
php:masterfrom
kairosci:feat/gh-22616-extension-child-init

Conversation

@kairosci

Copy link
Copy Markdown
Contributor

Fixes #22616 by adding per-extension child initialization hooks and invalidating inherited session GC randomness after forks. Validated with debug NTS and ZTS builds, shared extensions, nested forks, FPM worker recycling, and unavailable entropy sources.

Comment thread ext/session/session.c Resolved
@kairosci
kairosci requested a review from TimWolla October 26, 2026 21:43
Comment thread ext/session/session.c Resolved
@TimWolla
TimWolla requested a review from arnaud-lb October 26, 2026 21:54
@kairosci

Copy link
Copy Markdown
Contributor Author

The latest revision advances each child RNG state by a PID-derived distance without reseeding or changing the parent state. The reviewer confirmed this should work; all CI checks are green.

@arnaud-lb arnaud-lb left a comment

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.

Life cycles look ok: the seed is initialized once per thread from a CSPRNG, and advanced after a fork.

Please add new macros to declare and refer to the child init function, as we do for other hooks.

Comment thread ext/session/session.c Resolved
Comment thread ext/session/session.c Resolved
@kairosci
kairosci force-pushed the feat/gh-22616-extension-child-init branch from 9be2801 to 3d46385 Compare October 30, 2026 14:52
@kairosci

Copy link
Copy Markdown
Contributor Author

Addressed the review by adding macros to declare and reference child-init hooks. Fixed CI build failures by replacing obsolete atomic-store calls in zend_test; checks are rerunning.

@kairosci

Copy link
Copy Markdown
Contributor Author

Fixed the CI build failures by restoring the missing generated arginfo entries for both zend_test functions. The CI checks are running again.

Comment thread main/main.c Resolved
Comment thread Zend/zend_modules.h Resolved
@kairosci
kairosci force-pushed the feat/gh-22616-extension-child-init branch from 17bab85 to ec720cd Compare November 1, 2026 13:27
@kairosci
kairosci requested a review from derickr as a code owner November 1, 2026 13:27
@kairosci
kairosci force-pushed the feat/gh-22616-extension-child-init branch from ec720cd to ce8ba69 Compare November 1, 2026 13:28
Comment thread main/php.h Resolved
@kairosci
kairosci force-pushed the feat/gh-22616-extension-child-init branch from 3b86d34 to 3df5b71 Compare November 2, 2026 21:18
@arnaud-lb

arnaud-lb commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

About the STANDARD_MODULE_PROPERTIES_EX_WITH_CHILD_INIT() macro: It's fine now, but it will not scale if we add more hooks later (I plan to add at least one). I'm wondering if we could use named initializers instead:

  • Omit the new field child_init_func from STANDARD_MODULE_PROPERTIES_EX
  • Update STANDARD_MODULE_PROPERTIES_EX to use named initializers
  • Move the field before the ones initialized by STANDARD_MODULE_PROPERTIES_EX
  • Set child_init_func explicitly:
/* We can even omit the zero ones here: */
#define STANDARD_MODULE_PROPERTIES_EX \
    .module_started = 0, \
    .type = 0, \
    .handle = NULL, \
    .module_number = 0, \
    .build_id = ZEND_MODULE_BUILD_ID

struct _zend_module_entry {
	// ...
	zend_result (*post_deactivate_func)(void);
	void (*child_init_func)(void), // the new one
	int module_started;
	unsigned char type;
	void *handle;
	int module_number;
	const char *build_id;
};

zend_module_entry zend_test_module_entry = {
	STANDARD_MODULE_HEADER,
	"zend_test",
	ext_functions,
	PHP_MINIT(zend_test),
	PHP_MSHUTDOWN(zend_test),
	PHP_RINIT(zend_test),
	PHP_RSHUTDOWN(zend_test),
	PHP_MINFO(zend_test),
	PHP_ZEND_TEST_VERSION,
	PHP_MODULE_GLOBALS(zend_test),
	PHP_GINIT(zend_test),
	PHP_GSHUTDOWN(zend_test),
	NULL,
        .child_init_func = PHP_CINIT(zend_test),
	STANDARD_MODULE_PROPERTIES_EX,
};

The mix between named and unnamed is a bit ugly, so extensions that use child_init_func may switch to fully named initializers.

@TimWolla @kairosci thoughts?

@TimWolla

TimWolla commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

I'm wondering if we could use named initializers instead:

I'm a big fan of named initializers. I'm already annoyed when I want to add additional hooks after the fact and need to count which NULL to replace, because of course there is no comment either 😅

@kairosci

kairosci commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@arnaud-lb @TimWolla I updated to use named initializers. The field name makes the callback’s purpose clear, so no comment is needed.

@arnaud-lb

Copy link
Copy Markdown
Member

Thank you @kairosci. Please resolve #24647 (comment) and then we are good to merge. Also please rebase and re-run build/gen_stubs.php to resolve the conflicts.

@kairosci
kairosci force-pushed the feat/gh-22616-extension-child-init branch 4 times, most recently from f0853ab to e5c2efb Compare November 9, 2026 12:47
Fixes phpGH-22616.

Add zend_module_entry.child_init_func and invoke it from php_child_init()
for started modules in dependency order after core child initialization.
Expose PHP_CINIT/PHP_CINIT_FUNCTION macros and use named initializers for
the touched module properties.

In ext/session, seed GC randomness during GINIT and advance the PRNG state
in the child init hook so forked children do not share the same GC stream.
Keep session GC state in TLS globals.

Add zend_test coverage, a session fork regression test, and an FPM worker
recycling test. Regenerate zend_test arginfo.
@kairosci
kairosci force-pushed the feat/gh-22616-extension-child-init branch from e5c2efb to 3d5da1a Compare November 9, 2026 13:31
@kairosci

kairosci commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor Author

@TimWolla @arnaud-lb I think I've addressed all the reviews and conflicts.
Sorry again for missing some of them earlier!
Let me know if there's anything else I overlooked.

@arnaud-lb
arnaud-lb merged commit b9cf5d8 into php:master Nov 11, 2026
18 checks passed
@arnaud-lb

Copy link
Copy Markdown
Member

Thank you!

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.

Allow extensions to hook into php_child_init()

3 participants