The xinclude code from libxml removes the fallback node,
but the fallback node is still reference via $fallback.
The solution is to detach the nodes that are going to be removed in
advance.
Closes GH-14704.
Values retrieved from zend_getenv should be freed.
Note: The only possible value for `zend_getenv` is `sapi_getenv` which uses
zend alloc to duplicate the string that it reads from the SAPI module.
Closes GH-14708.
The error handling code isn't entirely right in two places.
One of the code blocks is dead because of an always-false condition, and
another code block is missing the assignment of a NULL pointer.
Getting the exact same behaviour is not entirely possible because you
can't extend the size of a shared memory region after it was made with
the Windows APIs we use, unless we destroy the region and recreate it,
but that has other consequences.
However, it certainly shouldn't crash.
Closes GH-14707.
The ping feature of php-fpm monitoring was previously not working
in pm.status_listen pool due to the configuration variables ping.path
and ping.response not being copied over to the worker when forked. This
results in the ping code path being disabled because the worker detects
that ping.path is not configured.
Closes GH-13980
Co-authored-by: Pierrick Charron <pierrick@php.net>
Although the issue was demonstrated using Curl, the issue is purely in
the streams layer of PHP.
Full analysis is written in GH-11078 [1], but here is the brief version:
Here's what actually happens:
1) We're creating a FILE handle from a stream using the casting mechanism.
This will create a cookie-based FILE handle using funopen.
2) We're reading stream data using fread from the userspace stream. This will
temporarily set a buffer into a field _bf.base [2]. This buffer is now equal
to the upload buffer that Curl allocated and note that that buffer is owned
by Curl.
3) The fatal error occurs and we bail out from the fread function, notice how
the reset code is never executed and so the buffer will still point to
Curl's upload buffer instead of FILE's own buffer [3].
4) The resources are destroyed, this includes our opened stream and because the
FILE handle is cached, it gets destroyed as well.
In fact, the stream code calls through fclose on purpose in this case.
5) The fclose code frees the _bs.base buffer [4].
However, this is not the buffer that FILE owns but the one that Curl owns
because it isn't reset properly due to the bailout!
6) The objects are getting destroyed, and so the curl free logic is invoked.
When Curl tries to gracefully clean up, it tries to free the buffer.
But that buffer is actually already freed mistakingly by the C library!
This also explains why we can't reproduce it on Linux: this bizarre buffer
swapping only happens on macOS and BSD, not on Linux.
To solve this, we switch to an unbuffered mode for cookie-based FILEs.
This avoids any stateful problems related to buffers especially when the
bailout mechanism triggers. As streams have their own buffering
mechanism, I don't expect this to impact performance.
[1] https://github.com/php/php-src/issues/11078#issuecomment-2155616843
[2] 5e566be7a7/stdio/FreeBSD/fread.c (L102-L103)
[3] 5e566be7a7/stdio/FreeBSD/fread.c (L117)
[4] 5e566be7a7/stdio/FreeBSD/fclose.c (L66-L67)
Closes GH-14524.
Although the issue mentioned FreeBSD, this is a broader problem:
the current ARM64 code to load the TLS offset assumes a setup with
the non-default TLS model. This problem can also apply on some
configurations on other platforms.
Closes GH-11236.
There's a hash table that maps type names to class name, but names with
a leading backslash are not supported. The engine has logic to strip
away the leading backslash that we should replicate here.
It works by checking if we need to make an actual copy in case an
unexpected (e.g. invalid data or leading backslash) situations are
detected. Upon making a copy we normalize the data in the table.
Furthermore, previously the code assumed that the key was always valid
and that the structure was a non-packed hash table. This isn't
necessarily the case. The new code fixes this as well.
Closes GH-14398.
There's a few leaks where the string is copied for lowercasing but not released.
Where possible, use the _lc functionality of zend_hash to do the lookup
to avoid the leaks that currently exist with the manual lowercasing.
Closes GH-14390.
zend_ini_long() actually expects the length without the NUL byte, but
we're passing the length *with* the NUL byte. This mess can actually be
avoided altogether by using INI_INT, so use that instead.
Closes GH-14382.
This partially backports that PR to stable branches as it has been in master
without reported problems so far.
It's only a partial backport because the stable branches don't have the
ZTS persistent resource fix that would fix shutdown crashes, i.e. the
code change in mysqlnd_vio's close_stream is not backported.
This is fully fixed on master.
Closes GH-14324.
Closes GH-10599.
If there is no root, the namespace cannot be attached to it,
so we have to attach it to the old list.
This isn't a problem in "new DOM" because namespaces are managed in a
separate structure there.
The incorrect functions are being called to deal with incoming
compressed data.
gzip/x-gzip corresponds to gzuncompress(), while deflate corresponds to
gzinflate().
The existing code for gzip compression also plays with removing the
first 10 bytes (i.e. the gzip header) to pass it to the inflate
implementation but that doesn't always work properly due to trailer
data. Get rid of that entirely by using the correct functions.
Closes GH-14321.
This fixes the incompatible pointer type warnings when checking for
reentrant functions declaractions (-Wincompatible-pointer-types) in
config.log. These were not declared on some obsolete systems if
_REENTRANT was not defined. The check is for now left in the code base
but can be transitioned to newer code without checking for missing
declarations or using these otherwise in the future.
Closes GH-14315.
Curl changed the behaviour, from the changelog:
- lib: make protocol handlers store scheme name lowercase curl/curl@c294f9c
From the docs: "The returned scheme might be upper or lowercase. Do
comparisons case insensitively."
Closes GH-14312.
Infallible in practice right now, but should be fixed as infallible today does not mean infallible tomorrow:
- sodium_crypto_sign_publickey_from_secretkey
- sodium_crypto_kx_seed_keypair
- sodium_crypto_kx_keypair
- sodium_crypto_auth
- sodium_crypto_sign_ed25519_sk_to_curve25519
- sodium_pad
Fallible today:
- sodium_crypto_sign_ed25519_pk_to_curve25519
Closes GH-14309.
php_pcre_replace_impl() can fail and return NULL. We should take that
error condition into account. Because other failures return false, we
return false here as well.
At first, I also thought there was a potential memory leak in the error
check of replacement_str, but found that the error condition can never
trigger, so replace that with an assertion.
Closes GH-14292.
Some modules may reset _fmode, which causes mangling of line endings.
Always be explicit like we do in other places where the native open call
is used.
Closes GH-14218.
Class constants are inherited to user classes without cloning. Thus, internal
class constants should not be persisted at all. Simply keep pointing to the
internal class constant.
Fixes GH-14109
Closes GH-14114
Fixes GH-13970
Closes GH-14105
We cannot validate at compile-time for multiple reasons:
* Evaluating the argument naively with zend_get_attribute_value can lead to code
execution at compile time through the new expression, leading to possible
reentrance of the compiler.
* Even if the evaluation was possible, it would need to be restricted to the
current file, because constant values coming from other files can change
without affecting the current compilation unit. For this reason, validation
would need to be repeated at runtime anyway.
* Enums cannot be instantiated at compile-time (the actual bug report). This
could be allowed here, because the value is immediately destroyed. But given
the other issues, this won't be needed.
Instead, we just move it to runtime entirely. It's only needed for
ReflectionAttribute::newInstance(), which is not particularly a hot path. The
checks are also simple.
libxml doesn't do reference counting inside its node types. It's
possible to remove an entity declaration out of the document, but then
entity references will keep pointing to that stale declaration. This
will cause crashes.
One idea would be to check when a declaration is removed, to trigger a
hook that updates all references. However this means we have to keep
track of all references somehow, which would be a high-overhead
solution. The solution in this patch makes sure that the fields are
always updated before they are read.
Closes GH-14089.
This is addon to the GH-13727 bug fix. When configuring the build with:
./configure CFLAGS=-Werror=strict-prototypes
libtool check for parsing nm command would fail:
checking command to parse /usr/bin/nm -B output from cc object... failed
Upstream libtool has this specific check already fixed. Note that this
works only with Autoconf version 2.72 and later and is preparation for
future compilers that might have this error enabled by default.
This is a backport of commit 03f15534a1 to
PHP-8.2 due to GH-14002 and fixes the PHP_CXX_COMPILE_STDCXX check in
ext/intl whether the specified C++ standard is mandatory or optional.
The `dnl` (Discard to Next Line) M4 macro in this combination of `m4_if`
macros and arguments isn't properly replaced and a literal `dnl` string
is appended in the configure script. The `[]dnl` works ok.
When step_callback fails, agg_context->val is passed dtor, but agg_context->val
is also used in final_callback regardless of the success/failure of step_callback,
so should not call dtor.
closes#14004fixes#13998
In the test cases, the compiler bails out due to a fatal error.
The data structures used by the compiler will contain stale values.
In particular, for the test case CG(loop_var_stack) will contain data.
The next compilation will incorrectly use elements from the previous
stack.
To solve this, we reset part of the compiler data structures.
We don't do a full re-initialization via init_compiler() because that will
also reset streams and resources.
Closes GH-13938.
php_socket_errno() may return a stale value when recv returns a
value >= 0. As such, the liveness check is wrong.
This is the same bug as #70198 (fixed in GH-1456). So we fix it in the
same way.
Closes GH-13895.
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 fixes the issue with unbounded waiting on SSL_peek which can happen
when only part of the record is fetched. It makes socket non blocking so
it is possible to verify if OpenSSL is expecting some more data or if
there is an error.
This also fixes bug #79501
Closes GH-13487
Inherited methods regardless of source must share the original runtime cache. Traits were missed.
This adds ZEND_ACC_TRAIT_CLONE to internal functions as well to allow easy distinction of these.
In commit 85e5635a, a feature test for the various libgd image formats
was added. That test however erroneously omits the GDLIB_CFLAGS (from
pkg-config) during compilation. This can lead to build failures and
therefore false negatives from the test.
Here, we add $GDLIB_CFLAGS to $CFLAGS for the duration of the test.
Closes GH-12019
autoconf/libtool generating code to test features missed `void` for
C calls prototypes w/o arguments.
Note that specific changes related to libtool have to be upstreamed.
Co-authored-by: Peter Kokot <petk@php.net>
close GH-13732
Regressed in 6fbf81c.
There is a missing error check on spl_filesystem_file_read_line(), which
means that if the line could not be read (e.g. because we're at the end
of the file), it will not set intern->u.file.current_line, which will
cause a NULL pointer deref later on.
Fix it by adding a check, and reintroducing the silent flag partially to
be able to throw an exception like it did in the past.
Closes GH-13692.
This is not just an issue due to missing initialization since moving the state
struct directly into the module globals. In earlier versions changing the mode
to `MT_RAND_PHP` within a single request would also affect the mode for
subsequent requests.
Original commit message follows:
This is a follow-up fix for GH-13579. The issue was detected in the nightly
MSAN build.
(cherry picked from commit bf0abd1629)
It's illegal to return from a bailout because that doesn't restore the
original bailout data. Return outside of it.
Test by YuanchengJiang
Closes GH-13689.
Inside `zend_object_std_dtor` the weakrefs are notified after the destruction
of properties already took place. In this test case, the destructor of an anon
class will be invoked due to the property destruction. That class has a
weak reference to its parent. This means that the destructor can access
parent properties that already have been destroyed, resulting in a UAF.
Fix this by notifying the weakrefs at the start of the object's
destruction.
Closes GH-13613.
the man page states `the locale facet is determined by the category argument, which should be
one of the LC_xxx constants defined in the <locale.h> header, excluding LC_ALL`,
since the 0.22.5 release, sanity checks had been strenghtened leading to
an abort with the Zend/tests/arginfo_zpp_mismatch.phpt test setting the
category to 0 which is LC_ALL on macOs.
close GH-13555
The character set ID included in the handshake data at the time of connection
actually only includes the lower 8 bits of the ID, so if try to use this to specify
a character set, the corresponding character set may not exist.
In case of an invalid character set, the default character set is now used
without an error.
Fixes#13452Closes#13470
There's a test that tries to make /etc world-writable, and asserts that
it fails. Although this test is guarded by a root user check, there are
situations where you don't need to be root to be able to do this.
This may thus have unwanted effects on your live filesystem.
The simple solution is to remove that part of the test. It doesn't
really add value anyway: we're trying to test the chmod error path, but
that exact same error path can be reached with any failure condition
that the kernel gives. For example, trying to chmod a non-existent file
will trigger the same code path.
While at it, also prefix the test path for the non-existent file such
that we don't accidentally modify the filesystem.
The chroot now has a better root-user check, that will not modify the
filesystem.
Other root-modifying mkdir tests were removed because they added no
value either.
Closes GH-13566.
PHP 8.1 and below interpreted unknown modes as `MT_RAND_MT19937`, but PHP 8.2+
interprets them as `MT_RAND_PHP`.
Align the behavior with PHP 8.1 and below, because folks should be steered
towards the standard mode.
When unserializing, the cached_resize field was not reset to -1
correctly, causing the setSize() method to think we were inside of a
resize operation.
Closes GH-13543.
The reflection failure is because the XML extension is used to check the
module dependency information, but that extension can be configured to
not depend on ext/libxml, resulting in a different output. The solution
is to check another extension instead.
The test failures in ext/xml/tests are because of different behaviour
between libxml2 and Expat error handling. These are expected differences
and the solution is to split the tests.
Closes GH-13522.
- Add tests
- NULL derefencing in read_dimension handler
- Fix isset()
- Fix empty() with column numbers as offsets
- Refactoring to use common functions
- Use gdFontCacheShutdown() to detect freetype
Currently we look for gdImageStringFT() to determine whether or not gd
has freetype support... but that function always exists. This leads
PHP to believe that gd has freetype support when it does not, and can
lead to build failures.
The gdFontCacheShutdown() function, on the other hand, is only present
when gd was built with freetype support. Let's use that instead.
- Fix GD image format detection
We currently check for, say, AVIF support by attempting to link a
program that calls libgd's gdImageCreateFromAvif() function. But
perversely, that function always exists in libgd; moreover when AVIF
support is missing it emits a warning and returns normally. Thus
our straightforward link test becomes not so straightforward.
This commit adds a new macro PHP_GD_CHECK_FORMAT that compiles, links,
and runs a test program instead. The test program overrides that "emit
a warning" handler so that the program actually fails if the format
we're looking for is not supported. This fixes detection of AVIF and
the other formats we check for in an external libgd.
- ext/gd/tests/bug77391.phpt: skip if gd lacks BMP support
I don't actually know how to remove BMP support from libgd, but PHP
has a ./configure test for it, so we should probably treat it as
optional.
Closes GH-12019
For master (8.4-dev) I merged GH-13381. But that PR changes public API
of TSRM, so cannot be used on lower branches.
This patch is a safe workaround for the issue, in combination with a
pre-existing fix using `ifdef ZTS + if (module_started)` inside pgsql
and odbc. The idea is to delay unloading modules until the persistent
resources are destroyed. This will keep the destructor code accessible
in memory.
This is not a proper fix on its own, because we still need the
workaround of not accessing globals after module destruction.
The proper fix is in master.
Closes GH-13388.
php_array_key_compare_string_case_unstable_i has a typo for the second
operand resulting in a wrong buffer size calculation.
Issue reported by @AlexRudyuk
Close GH-13315
libgd uses an incorrect absolute path check in gdft.c.
It checks if either the path starts with a '/' (only valid on Posix
btw), or whether it contains something of the form C:\ or C:/.
However, this overlooks the possibility of using UNC paths on Windows.
As we already do PHP-specific stuff with VCWD_ macros, use
IS_ABSOLUTE_PATH to check for an absolute path which will take into
account UNC paths as well.
Closes GH-13241.
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.
Commit 5cbe5a538c disabled chunking for all writes to streams. However,
user streams have a callback where code is executed on data that is
subject to the memory limit. Therefore, when using large writes or
stream_copy_to_stream/copy the memory limit can easily be hit with large
enough data.
To solve this, we reintroduce chunking for userspace streams.
Users have control over the chunk size, which is neat because
they can improve the performance by setting the chunk size if
that turns out to be a bottleneck.
In an ideal world, we add an option so we can "ask" the stream whether
it "prefers" chunked writes, similar to how we have
php_stream_mmap_supported & friends. However, that cannot be done on
stable branches.
Closes GH-13136.
Fixed an issue where pdo_firebird float and double type values were wrong.
Changed from using `%F` format with `zend_strpprintf` to using `%H` format with
`zend_strpprintf_unchecked`.
Fixes GH-13119
Closes GH-13125
Global --tag=CC defined in configure.ac is not correct in all cases. For example
linking objects that were compiled from C++ sources needs to be done with C++
compiler, however for link mode libtool will prefer compiler indicated with
--tag.
Fixes GH-12349
Issue occur when compiling with recent clang releases (> 13) and
with the '-Os' optimisation level, after using
`imageloadfont` which returns a proper GdFont class leads to
a subtle bug when attempting to use via the imagefont* function.
We should perhaps look into a generic system to ask the SAPI whether
a feature should be supported or not. Or, we should look into making
a denylist instead of an allowlist.
Anyway, let's not try doing anything fancy on stable branches.
Closes GH-13070.
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.
On ZTS, the global variables are stored in dynamically allocated memory.
When the module gets shut down this memory is released. After the module
is shut down, only then are the persistent resources cleared. Normally
this isn't an issue, but pgsql and odbc refer to the globals to modify
some counters, after the globals have been freed.
Fix this by guarding the modification.
Closes GH-13032.
Autotools emits warning if 3rd argument is empty. Call is wrapped in the
AC_CACHE_CHECK with php_cv_* cache variable name according to the docs.
Closes GH-12966
Because these functions are copied and not properly registered (which we
can't), the observer code doesn't add the temporaries on startup.
Add them via a callback during startup.
Closes GH-12906.
This is a false positive. The cycle detection code stops at 10.000
iterations. Instead of stopping at a fixed amount, make it more robust
by implementing Floyd's cycle detection algorithm.
Closes GH-12954.
It seems SELinux has a bug where memory directly adjacent to the heap is
interpreted as heap memory. Dodge this issue by leaving some space between the
heap and memory suggested by find_prefered_mmap_base.
See GH-12932
See https://bugzilla.kernel.org/show_bug.cgi?id=218258
Closes GH-12942
There's two problems:
- Some loops used `unsigned int` instead of `size_t`.
- The 2*N-bit addition that is emulated using 2 N bit numbers has a bug:
it first truncated the number to 32/64 bit and only then shifted. This
resulted in the wrong length info stored inside the resulting hash.
Closes GH-12937.
When the cli-server specifies a router script, we run it using
zend_execute_scripts instead of php_execute_script, because the former preserves
the return value of the script. However, php_execute_script also starts resets
the execution timer with the value from max_execution_time. If the timer has
previously been initialized with max_input_time, it will never be reset, and
thus trigger at the incorrect time.
Closes GH-12886
The issue referenced here doesn't contain a reproducer, but I recently
received an email of a user with the exact same problem. I was able to
recreate the scenario locally using vsftpd and setting
`require_ssl_reuse=YES` in the vsftpd configuration.
It turns out that our session resumption code is broken. It only works a
single time: the first time a data connection opens. Subsequent data
connections fail to reuse the session. This is because on every data
connection a new session is negotiated, but the current code always
tries to reuse the (stale) session of the control connection.
To fix this, we use SSL_CTX_sess_set_new_cb() to setup a callback that
gets called every time a new session is negotiated. We take a strong
reference using SSL_get1_session() and store it in the ftpbuf_t struct.
Every time we open a data connection we'll take that session.
This works because every control connection has at most a single
associated data connection.
Also disable internal session caching storage to not fill the cache up
with useless sessions.
There is no phpt for this because PHP does not support enforcing SSL
session reuse.
It is however testable manually by setting up vsftpd and setting the
`require_ssl_reuse=YES` function from before.
Closes GH-12851.
If there are two users that can execute the script that caches a WSDL,
but the script is owned by a single user, then the caching code will
name the cached file with the file owner username and a hash of the uri.
When one of the two tries to rename the file created by the other
process, this does not work because it has no permission to do so.
This then leaves temporary files floating in the temp directory.
To fix the immediate problem, unlink the file after rename has failed.
On the long term, this has to be fixed by taking the username of the
process instead of the username of the file owner.
Closes GH-12841.
php_strip_url_passwd modifies url in-place. We cannot assume from
php_message_handler_for_zend that data is a temporary, modifiable string.
Fixes oss-fuzz #64209
Closes GH-12733
The namespace data is freed and set to NULL, but there remain references
to the namespace declaration nodes. This (rightfully) confuses libxml2
because its invariants are broken. We also have to remove all remaining
references from the subtree. This fixes the data corruption bug.
Closes GH-12681.
Have to use file_put_contents() instead of --FILE-- because we have to
actually load it using the exec command, *and* have to make multiple
files, and note that we can only load files relative from the current
directory, so we can't rely on files being in the sapi/phpdbg/tests
folder.
Closes GH-12680.
The temporary HashTable has a destructor that releases the string held
by the entry's value. However, browscap_intern_str(_ci) only incremented
the refcount for the reference created by the return value. As the
HashTable is only used during parsing, we don't need to manage the
reference count of the value anyway, so get rid of the destructor.
This is triggerable in two cases:
- When using php_admin_value to set the ini at the activation stage
- When running out of space for the opcache-interned strings
Closes GH-12634.
php_strip_url_passwd modifies url in-place. We cannot assume from
php_message_handler_for_zend that data is a temporary, modifiable string.
Fixes oss-fuzz #64209
Closes GH-12733
The namespace data is freed and set to NULL, but there remain references
to the namespace declaration nodes. This (rightfully) confuses libxml2
because its invariants are broken. We also have to remove all remaining
references from the subtree. This fixes the data corruption bug.
Closes GH-12681.
Have to use file_put_contents() instead of --FILE-- because we have to
actually load it using the exec command, *and* have to make multiple
files, and note that we can only load files relative from the current
directory, so we can't rely on files being in the sapi/phpdbg/tests
folder.
Closes GH-12680.
The temporary HashTable has a destructor that releases the string held
by the entry's value. However, browscap_intern_str(_ci) only incremented
the refcount for the reference created by the return value. As the
HashTable is only used during parsing, we don't need to manage the
reference count of the value anyway, so get rid of the destructor.
This is triggerable in two cases:
- When using php_admin_value to set the ini at the activation stage
- When running out of space for the opcache-interned strings
Closes GH-12634.
Removes 'PRAGMA writable_schema;'.
From sqlite changelog https://www.sqlite.org/releaselog/3_44_0.html:
> The SQLITE_DBCONFIG_DEFENSIVE setting now prevents PRAGMA writable_schema
> from being turned on.
> Previously writable_schema could be turned on, but would not actually allow
> the schema to be writable. Now it simply cannot be turned on.
Closes GH-12636.
Setting the stream context via php_stream_context_to_zval() will
increase the reference count. So if the new context is created, then it
will end up with a reference count of 2 while it should be 1.
Credits to cmb for the analysis. I arrived at the same patch as he did.
Closes GH-12523.
