SAPI: Convert sapi_getenv to zend_string - #23386
Conversation
19195ea to
94c08df
Compare
LamentXU123
left a comment
LamentXU123
left a comment
There was a problem hiding this comment.
Phar part. I am in favor of this.
| entry = path_info; | ||
| if (zs_path_info) { | ||
| entry = estrdup(ZSTR_VAL(zs_path_info)); | ||
| entry_len = strlen(entry); |
There was a problem hiding this comment.
This can just be
| entry_len = strlen(entry); | |
| entry_len = ZSTR_LEN(zs_path_info); |
There was a problem hiding this comment.
estrdup will cut off after the first NUL byte, so if there's an embedded NUL byte then entry_len will go out of bounds. Can't happen with environment variables on their wn, but perhaps via input filters it can
There was a problem hiding this comment.
:| welp I hope that input filter can't mess up like that but this is PHP we're talking about. Might make sense to guard strings that have nul bytes in them as that seems potentially problematic.
There was a problem hiding this comment.
FWIW, I do have a draft branch (not committed/pushed yet) for trying to make input filters use zend_string as well (since it's ugly in sapi_getenv and also the API of the filters are as well), but it's a mild slog that touches a surprising amount of stuff.
For the phar path mangling here, this should also just be zend_stringified the whole way through. Doable, just annoying
There was a problem hiding this comment.
I was working on converting char* to zend_string in phar and there might even still be a PR somewhere that does this (or somewhere on my local machine) but those conversions can be quite tedious yeah.
| ret = getenv(name); | ||
| } | ||
| return ret ? zend_string_init(ret, strlen(ret), 0) : NULL; | ||
| #else |
There was a problem hiding this comment.
I don't understand the logic. But seems like you've missed something here? if ret is not empty we should use ret, this is also true in the else branch.
There was a problem hiding this comment.
I forgot the logic in the Windows ifdef, that should be fixed.
The old char* return was very ambigious at the SAPI level about ownership per platform (unix returned environ buffer, Win32 returned converted buffer), which it then estrduped at the SAPI frontend level. To clarify this, instead return an allocated zend_string in the SAPIs which we bubble up to consumers. There are some annoyances still; input filters have to shuffle some buffers because input filters expect to work on raw buffers, and phar does some mildly harrowing string mangling without zend_string. These should be cleaned up. There is also the question if SAPIs just bubbling up getenv when they lack a web server specific context i.e. from fcgi or Apache (CGI, FPM, Litespeed), as this seems not quite the right intent; CLI doesn't implement a getenv function for instance.
94c08df to
fc20723
Compare
|
Addressed your comments. |
4 participants
The old char* return was very ambigious at the SAPI level about ownership per platform (unix returned environ buffer, Win32 returned converted buffer), which it then estrduped at the SAPI frontend level. To clarify this, instead return an allocated zend_string in the SAPIs which we bubble up to consumers.
There are some annoyances still; input filters have to shuffle some buffers because input filters expect to work on raw buffers, and phar does some mildly harrowing string mangling without zend_string. These should be cleaned up.
There is also the question if SAPIs just bubbling up getenv when they lack a web server specific context i.e. from fcgi or Apache (CGI, FPM, Litespeed), as this seems not quite the right intent; CLI doesn't implement a getenv function for instance.