Home Original page

[fix segfault] ensure environment trackvars are setup by withinboredom · Pull Request #1799 · php/frankenphp

@withinboredom

@withinboredom 

Signed-off-by: Robert Landers <landers.robert@gmail.com>

@withinboredom withinboredom changed the title [fix] ensure environment trackvars are setup [fix segfault] ensure environment trackvars are setup

Aug 10, 2025

@withinboredom 

Signed-off-by: Robert Landers <landers.robert@gmail.com>

@AlliBalliBaba

Yeah you are right, $_ENV is currently weird in worker mode since we are clearing the global environment between requests, but not re-registering it again.

That makes it so $_ENV is still available in the global symbols table, but the global itself is gone.

I think the better solution would be to not clear the global here, but then PHP will complain about memory leaks. Maybe it would work if the whole hashtable is marked as 'permanent'

@withinboredom

Marking it as permanent just means we’re responsible for freeing it and it won’t be tracked for memory leaks. The memory leaks still may be there, but php won't report it.

@AlliBalliBaba

Hmm I wonder if it would also be possible to allocate the hashtable once per process and share it across all threads. Though I'm not sure if it could be guaranteed that PHP doesn't modify it internally.

@withinboredom

Hmm I wonder if it would also be possible to allocate the hashtable once per process and share it across all threads. Though I'm not sure if it could be guaranteed that PHP doesn't modify it internally.

Not really. Everything in it would also have to be permanent as variable lifetimes are tied to the request — closures and objects can’t be shared across requests at all, at least in my experiments. OPcache does this with class entries and other things by storing it in an SHM. Being that frankenphp has much easier to share memory, it is probably possible to speed up OPcache by replacing that part with a frankenphp-friendly implementation.

dunglas

func addPreparedEnvToServer(fc *frankenPHPContext, trackVarsArray *C.zval) {
for k, v := range fc.env {
C.frankenphp_register_variable_safe(toUnsafeChar(k), toUnsafeChar(v), C.size_t(len(v)), trackVarsArray)
C.frankenphp_register_env_safe(toUnsafeChar(k), toUnsafeChar(v), C.size_t(len(v)))

Choose a reason for hiding this comment

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

Can't we pass an additional flag to frankenphp_register_variable_safe() instead of introducing a new function? This would avoid a lot of cgo calls.

By the way, even if unrelated, we could do a single call to frankenphp_register_variable_safe() by looping C-side for the same reason (likely a micro-optimization, but still).

AlliBalliBaba

}
}

void frankenphp_register_env_safe(char *key, char *val, size_t val_len) {

Choose a reason for hiding this comment

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

Not sure if it's a good idea to register $_ENV together with $_SERVER. $_ENV should be left empty if there is no "E" in the variables order ini directive Wouldn't you achive something similar by just calling (haven't tried)

zend_is_auto_global(ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_ENV))

Right after we call

zend_is_auto_global(ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_SERVER))

Choose a reason for hiding this comment

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

I'm not sure what you mean, AFAICT, php always calls these for environment variables. Other things use this underlying plumbing (such as xdebug). Enabling $_ENV actually copies this array to $_ENV (at least at the code I looked at) in other SAPIs.

Choose a reason for hiding this comment

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

Ohh... I see what you're saying. I'll give that a gander as well.

Choose a reason for hiding this comment

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

What also might be worth a try is resetting the PG(http_globals) manually instead of calling

@dunglas

Closing in favor of #1814.