[fix segfault] ensure environment trackvars are setup #1799
Conversation
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
|
Yeah you are right, 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' |
|
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. |
|
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. |
| 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))) |
There was a problem hiding this comment.
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).
| } | ||
| } | ||
|
|
||
| void frankenphp_register_env_safe(char *key, char *val, size_t val_len) { |
There was a problem hiding this comment.
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))There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ohh... I see what you're saying. I'll give that a gander as well.
There was a problem hiding this comment.
What also might be worth a try is resetting the PG(http_globals) manually instead of calling
Line 216 in c7bc5a3
|
Closing in favor of #1814. |
I’ve been chasing down this issue for MONTHS. I finally managed to figure out the issue.
In workers, $_ENV works EXTREMELY weird sometimes, depending on where you access it from. If you accessed in the global scope, you could get different values than if you access it in a function or method.
Well... I checked
PG(http_globals)[TRACK_VARS_ENV]in an extension (expecting to get the real env) and caused a segfault because it isn’t set until after the first request (and it is always empty).This PR fixes the issue by making sure the http_globals array stays in sync with the environment.
potentially related PRs:
xdebug_connect_to_client()is called in worker mode #1504$_ENVgets overridden #1061