Conversation
henderkes
force-pushed
the
fix/1674
branch
2 times, most recently
from
d1b2072 to
129303e
Compare
keep track of separate prepared_env set for a php_server correctly exposes env vars into $_ENV when E is in `variables_order` also fixes the problem of lazy server eval I didn't think about last commit
| } | ||
| } | ||
|
|
||
| func addPreparedEnvToServer(fc *frankenPHPContext, trackVarsArray *C.zval) { |
There was a problem hiding this comment.
I think you could do this now in a single Cgo call, since the prepared env is already on the C side
There was a problem hiding this comment.
I think that complicates things and falls outside the scope of the PR. Not saying we shouldn't do it, but if we do, it should be for both prepared_env and sandboxed_env at once, after this one is merged.
There was a problem hiding this comment.
It would just be a call to zend_array_copy, but it can be done in a separate PR if you want to. Only for the prepared_env, not the sandboxed_env
| if (sandboxed_env != NULL) { | ||
| return zend_hash_str_find(sandboxed_env, name, name_len); | ||
| } | ||
|
|
||
| zval *env_val = NULL; | ||
| if (prepared_env != NULL) { | ||
| env_val = zend_hash_str_find(prepared_env, name, name_len); | ||
| } |
There was a problem hiding this comment.
This would always ignore all prepared envs if any sandboxed env is set
There was a problem hiding this comment.
Yes, as a small performance optimization. When sandboxed_env is set up from main_thread env, I directly overlay prepared_env into it. Could get confusing if we build the sandboxed env and forget to overlay it though.
There was a problem hiding this comment.
Hmm no I mean if i have key1=value1 in prepared env. Then I do putenv(key2,=value2), then getenv(key1) would not return value1 anymore. Unless I'm missing something
There was a problem hiding this comment.
What happens is this:
request comes in
- main_thread_env is key=main_value
- sandboxed_env is empty
- prepared_env is key=value1
retrieving key
- sandboxed env is empty
- getenv() -> uses prepared_env, returns value1
- $_ENV -> returns value1
user calls putenv(key=value2)
- sandboxed env is initialised based on main_thread_env
- prepared_env overwrites the main_thread_env stuff inside sandboxed_env
- putenv overwrites key to value2
retrieving key again
- getenv -> reads sandboxed_env, which exists now, returns value2
- $_ENV -> not repopulated by putenv, still returns value1
And if putenv didn't overwrite the key, the old one, copied from prepared_env, would persist too.
| if (sandboxed_env != NULL) { | ||
| return zend_array_dup(sandboxed_env); | ||
| } | ||
|
|
||
| HashTable *env = zend_array_dup(main_thread_env); | ||
| if (prepared_env != NULL) { | ||
| zend_hash_copy(env, prepared_env, (copy_ctor_func_t)zval_add_ref); | ||
| } | ||
|
|
||
| return env; |
There was a problem hiding this comment.
Also here the prepared env is ignored if any sandboxed env is set. Would be a very subtle bug, but maybe this needs a test as well.
There was a problem hiding this comment.
Yeah, not a bug, but you're right, we need a test as it's too easy to accidentally set sandboxed_env without accounting for prepared_env.
There was a problem hiding this comment.
Added tests coving every angle here, including worker mode where $_ENV isn't populated regardless of variables_order. I remember this being discussed and we decided on not changing worker mode, cba digging up the issue now.
|
Tbh the name 'prepared_env' has always confused me. "request_specific_env", "request_local_env", "request_context_env", "request_env" or something similar would make more sense IMO |
Prepared as in prepared by the |
2 participants
closes #1674
was looking through old issues and found this one that already had a plan to solve it