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.
This works, but UBSan running on a big endian platform (in this, ppc64)
will complain that the ((uchar*)buffer)[n] is int, and shifting that
could be weird. Since the value of PHAR_GET_32 et al are almost always
unsigned, it makes sense to cast these as unsigned.
Fixes phar tests on a big endian system with UBSan enabled.
Regressed in 1facbc385a
The macro caused the value to be read over and over, but the change to
a variable makes the condition not work like before. Just inline the
read.
When a class (or enum) has no methods, rather than using an array that only
contains `ZEND_FE_END`, use `NULL` for the functions. The implementation of
class registration for internal classes, `do_register_internal_class()` in
zend_API.c, already skips classes where the functions are `NULL`. By removing
these unneeded arrays, we can reduce the size of the header files, while also
removing an unneeded call to zend_register_functions() for each internal class
with no extra methods.
Niels Dossche