ext/phar: Refactor Phar to prevent unnecessary strlen() computations #11033
Conversation
There was a problem hiding this comment.
Looks good.
@iluuu1994 please also take a short look.
I didn't understand the removed code after zend_throw_exception_ex(). Note, it just set EG(exception) and doesn't perform C stack unwinding.
| if (path && path == entry) { | ||
| efree(entry); | ||
| } | ||
|
|
||
| if (arch) { | ||
| efree(arch); | ||
| } | ||
|
|
||
| goto finish; |
There was a problem hiding this comment.
The code outside the if branch is equivalent to this and performs the necessary clean-up and then jumps to finish
| @@ -256,17 +256,23 @@ zend_string *phar_find_in_include_path(zend_string *filename, phar_archive_data | |||
| return NULL; | |||
| } | |||
|
|
|||
| fname = (char*)zend_get_executed_filename(); | |||
| fname_len = strlen(fname); | |||
| zend_string *fname = zend_get_executed_filename_ex(); | |||
There was a problem hiding this comment.
Contrary to zend_get_executed_filename() zend_get_executed_filename_ex() may return NULL. I don't know if this code can be triggered outside of user context. The same goes for the calls above.
| return SUCCESS; | ||
| } | ||
|
|
||
| if (!strcmp(fname, "[no active file]")) { | ||
| if (zend_string_equals_literal(fname, "[no active file]")) { |
There was a problem hiding this comment.
This no longer makes sense, this string is only returned in zend_get_executed_filename(). This would now return NULL (and thus already segfault above).
There was a problem hiding this comment.
Ah indeed, surprised we don't have a test for that... AFAIK this only happens with eval()?
Based on top of #11032
We also make all checks for the stream wrapper protocol case-insensitive.
Minor drive-by code clean-up.