Conversation
This char* is derived from the fname char* field.
As this is derived from a live char* pointer where we don't have the ownership.
| /* The ext field stores the location of the file extension from the fname field, and thus should never be freed. */ | ||
| uint32_t ext_len; | ||
| char *ext; | ||
| const char *ext; |
There was a problem hiding this comment.
Do you actually need to store the ext string, or do you only need the length? Memory and gut tells me you only need the length
There was a problem hiding this comment.
Haven't fully looked into this, as I mainly wanted to split this change out of #21823 :) but happy to look into it to double-check if it is needed as this whole thing does seem slightly dubious
There was a problem hiding this comment.
I guess the one case that requires closer investigation is the usage of phar_detect_phar_fname_ext() in phar_rename_archive() as this sets the ext field. But it does seem indeed like neither the ext_len nor ext are really used for anything?
Makes me think that refactoring phar_detect_phar_fname_ext() to return an enum of PATH_IS_URL|USE_ALIAS|IS_TAR|IS_ZIP|IS_PHAR might be more sensible.
Anyhow, I'll merge the PR as is to unblock the fname to zend_string conversion, but will definitely have another look at this ext field mess :)
2 participants
No description provided.