Fix GH-16695: phar:// tar parser and zero-length file header blocks #16700
Conversation
ext/phar/tests/gh16406
Outdated
There was a problem hiding this comment.
@nielsdos This one likely does not belong here, right?
There was a problem hiding this comment.
Indeed, this is an accident, good catch! I've also updated one of the tests and added an extra test according to your suggestion.
There are two issues: 1) There's an off-by-one in the check for the minimum file size for a tar (i.e. `>` instead of `>=`). 2) The loop in the tar parsing parses a header, and then unconditionally reads the next one. However, that doesn't necessarily exist. Instead, we remove the loop condition and check for the end of the file before reading the next header. Note that we can't use php_stream_eof as the flag may not be set yet when we're already at the end.
| --CREDITS-- | ||
| hakre |
There was a problem hiding this comment.
Have read this in CONTRIBUTING.md:
For newly created tests, a
--CREDITS--section should no longer be included,
as test authorship is already accurately tracked by Git. If multiple authors
should be credited, theCo-authored-bytag in the commit message may be used.
ref: 7514b5d ("Mention that zend_parse_parameters should not be tested", 2019-07-07)
at least fyi @nielsdos , if you consider this and you need a user/email for the Co-authored-by trailer: Hans Krentel (hakre) <[email protected]>
There was a problem hiding this comment.
Yeah I know the CONTRIBUTING doc says that, but I find it a stupid rule because the Co-authored-by tag is not granular. (it's barely a rule imo due to the weak wording). However, a big chunk of the patch is tests of yours so I'll add a Co-authored-by tag upon merging.
| @@ -607,7 +611,7 @@ int phar_parse_tarfile(php_stream* fp, char *fname, size_t fname_len, char *alia | |||
| phar_destroy_phar_data(myphar); | |||
| return FAILURE; | |||
| } | |||
| } while (!php_stream_eof(fp)); | |||
There was a problem hiding this comment.
Isn't it a problem then that the eof test has been removed? Here now 261: pos = php_stream_tell(fp); pos is unchecked for error returns (are there any?; was unchecked before, too).
Probably can be moved to now 258 while (true) { as while (!php_stream_eof(fp)) { ? @nielsdos
There was a problem hiding this comment.
php_stream_tell(fp) cannot fail.
while (!php_stream_eof(fp)) { won't work because the flag is not set in time, leading to the same issue as before.
There was a problem hiding this comment.
@nielsdos Thank you for the clarification.
There are two issues:
tar (i.e.
>instead of>=).reads the next one. However, that doesn't necessarily exist.
Instead, we remove the loop condition and check for the end of the
file before reading the next header. Note that we can't use
php_stream_eof as the flag may not be set yet when we're already at
the end.