PharFileInfo just takes a pointer from the manifest without refcounting
anything. If the entry is then removed from the manifest while the
PharFileInfo object still exists, we get a UAF.
We fix this by using the fp_refcount field. This is technically a
behaviour change as the unlinking is now blocked, and potentially file
modifications can be blocked as well. The alternative would be to have a
field that indicates whether deletion is blocked, but similar corruption
bugs may occur as well with file overwrites, so we increment fp_refcount
instead.
This also fixes an issue where a destructor called multiple times
resulted in a UAF as well, by moving the NULL'ing of the entry field out
of the if.
Closes GH-17811.
Commit edae2431 attempted to fix a leak and double free, but didn't
properly understand what was going on, causing a reference count mistake
and subsequent segfault in this case.
The first mistake of that commit is that the reference count should've
been increased because we're reusing a phar object. The error handling
path should've gotten changed instead to undo this refcount increase
instead of not refcounting at all (root cause of this bug).
The second mistake is that the alias isn't supposed to be transferred or
whatever, that just doesn't make sense. The reason the test
bug69958.phpt originally leaked is because in the non-reuse case we
borrowed the alias and otherwise we own the alias. If we own the alias
the alias information shouldn't get deleted anyway as that would desync
the alias map.
Fixing these will reveal a third issue in which the alias memory is not
always properly in sync with the persistence-ness of the phar, fix this
as well.
Closes GH-17150.
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.
Closes GH-16700.
When copying entries during conversion in phar_convert_to_other(), the
header offset is not reset. This didn't matter in the past as it wasn't
used anyway in the particular use-case, but since 1bb2a4f9 this is
actually used and sanity-checked.
Closes GH-16470.
MAPPHAR_FAIL will call the destructor of the manifest, mounted_dirs, and
virtual_dirs tables. When a new phar object is allocated using (p)ecalloc,
the bytes are zeroed, but the flag for an uninitialized table is
non-zero. So we have to manually set the flag in case that we have a
code path that can destroy the tables without first initializing them at
least once.
Closes GH-13847.
If the destination already exists, then the `add` function on the
manifest will return NULL, resulting in a NULL entry and therefore a
NULL deref. As `copy()` (not `Phar::copy`) chooses to succeed and
overwrite the destination if it already exists, we should do the same.
Therefore the fix is as simple as changing `add` to `update`.
Closes GH-13840.
This also fixes skipped tests due to different naming "zend-test"
instead of "zend_test" and "PDO" instead of "pdo":
- ext/dom/tests/libxml_global_state_entity_loader_bypass.phpt
- ext/simplexml/tests/libxml_global_state_entity_loader_bypass.phpt
- ext/xmlreader/tests/libxml_global_state_entity_loader_bypass.phpt
- ext/zend_test/tests/observer_sqlite_create_function.phpt
EXTENSIONS section is used for the Windows build to load the non-static
extensions.
Closes GH-13276
The code currently assumes that the extra field length of the central
directory entry and the local entry are the same, but that's not the
case. For example, the "Extended Timestamp extra field" differs in size
for local vs central directory entries. This causes the file contents
offset to be incorrect because it is based on the central directory
length instead of the local entry length. Fix it by reading the local
entry and getting the size from there as well as checking consistency
for the file name length.
Closes GH-13045.
- For Windows we just have to set the right error_reporting value
- Test cannot be used repeatedly on Opcache because the unlink will have
no effect because of caching.
Closes GH-13129.
phar_get_pharfp() can return NULL. In this case this is because the
stream gets closed by the include code in the engine. However, the phar
entry is still cached, so when the next include happens the engine tries
to read from a closed (and nullified) stream.
Use the same fix as in phar_open_entry_fp(): take into account that the
phar_get_pharfp() can return NULL and in that case reopen the phar
archive.
Closes GH-13056.
Cirrus will no longer offer unlimited free builds starting next month. We don't
have an alternative for FreeBSD and ARM, so move what we can for now.
Closes GH-11898
Commit 0b2e6bc2b0 started caching the directory entry type to improve
performance. Shortly after, we've seen flaky failures of the
buildFromIterator phar test.
When it fails, it's always a value error in the constructor of
RecursiveDirectoryIterator::__construct() with a "no such file or
directory" error. What's happening here is this:
1) A parallel test creates a subdirectory in the current working dir.
2) This test checks hasChildren() on a directory entry, the cached entry
returns "yes" on the subdirectory.
3) The parallel test finishes and removes the subdirectory.
4) The constructor mentioned above is called, causing an exception
because the directory is gone.
This race has always been possible, even before said commit. It's just
that it was very hard to hit before: the expensive stat call made the
race window hard to hit. The race is now easier to hit because of the
caching that is fast.
Since there's many tests that modify the current working directory, it
seems best to mark this as an "all" conflict. We cannot avoid every
TOC-TOU race when working with files with these phar tests.
In particular, mounteddir.phpt caused every conflict I saw on CI, but
there's more tests that create subdirectories in the current working
directory.
Closes GH-11869.
