Fix #79503: Memory leak on duplicate metadata #5431
Conversation
As you can only end up with duplicate metadata if you explicitly tamper with the phar (presumably), I don't think we need to be concerned about BC here. That said, it doesn't seem particularly important to me whether we make it just a warning or not. |
|
How about warning in PHP 7.x, and Error in master? |
|
Looking a bit closer at the code, it seems to pretty consistently handle errors by populating |
Duplicate metadata can only happen if someone tampers with the phar, so we can and should treat that as error.
Well, then duplicate metadata would constitute a corruption, so we better bail out. I don't think it's necessary to have a special error message in this case; "tar-based phar … has invalid metadata in magic file …" seems to be good enough. |
|
| @@ -181,9 +181,15 @@ static int phar_tar_process_metadata(phar_entry_info *entry, php_stream *fp) /* | |||
| } | |||
|
|
|||
| if (entry->filename_len == sizeof(".phar/.metadata.bin")-1 && !memcmp(entry->filename, ".phar/.metadata.bin", sizeof(".phar/.metadata.bin")-1)) { | |||
| if (Z_TYPE(entry->phar->metadata) != IS_UNDEF) { | |||
There was a problem hiding this comment.
We should do this check earlier, before we parse, I think.
There was a problem hiding this comment.
Wouldn't that require to duplicate the check whether the metadata belong to an entry or the whole phar (or to remember the result of that check)? Maybe it's more straight forward to free the parsed metadata before returning?
|
Thanks! Applied as ccca2c4. |
This is the minimal fix for the mem leak.
@nikic wrote:
It seems to me that reporing an error instead would be a BC break. Maybe better to just raise a notice/warning in addition, but keep overwriting metadata like now.