atoi()'s return value is actually undefined when an underflow or
overflow occurs. For example on 32-bit on my system the overflow test
which inputs "h2147483648" results in repetitions==2147483647 and on
64-bit this gives repetitions==-2147483648. The reason the test works on
32-bit is because there's a second undefined behaviour problem:
in case 'h' when repetitions==2147483647, we add 1 and divide by 2.
This is signed-wrap undefined behaviour and accidentally triggers the
overflow check like we wanted to.
Avoid all this trouble and use strtol with explicit error checking.
This also fixes a semantic bug where repetitions==INT_MAX would result
in the overflow check to trigger, even though there is no overflow.
Closes GH-10943.
The recent clang-16 throws errors for implicitly defined functions by
default. In many ./configure tests, an undefined function (which is
"implicitly defined" when you try to call it) is undefined because it
really does not exist. But in one case, utf8_to_mutf7() is undefined
because we forgot to include the header that defines it.
This commit updates the test for utf8_to_mutf7:
* We now include the header (c-client.h) that defines it.
* A "checking... yes/no" message was added to the test.
* The test was switched from PHP_IMAP_TEST_BUILD to AC_COMPILE_IFELSE.
This was the easiest way to avoid a return-type mismatch that runs
afoul of -Werror=implicit-int.
* CPPFLAGS is temporarily amended with the -I flag needed to find
c-client.h.
Fixes GH-10947.
Closes GH-10948
Signed-off-by: George Peter Banyard <girgias@php.net>
The alignment of sqldata is in most cases only the basic alignment,
so the code type-puns it to a larger type, it *can* crash due to the
misaligned access. This is only an issue for types > 4 bytes because
every sensible system requires an alignment of at least 4 bytes for
allocated data.
Even though this patch uses memcpy, the compiler is smart enough to
optimise it to something more efficient, especially on x86.
This is just the usual approach to solve these alignment problems.
Actually, unaligned memory access is undefined behaviour, so even on x86
platforms, where the bug doesn't cause a crash, this can be problematic.
Furthermore, even though the issue talks about a 64-bit kernel and
32-bit userspace, this doesn't necessarily need to be the case to
trigger this crash.
Test was Co-authored-by: rvk01
Closes GH-10920.
The stream context inside `mysqlnd_vio::enable_ssl()` is leaking.
In particular: when `php_stream_context_set()` get called the refcount
of `context` is increased by 1, which means that `context` will now
have a refcount of 2. Later on we remove the context from the stream
by calling `php_stream_context_set(stream, NULL)` but that leaves our
`context` with a refcount of 1, and therefore it's never destroyed.
In my test case this yielded a leak of 1456 bytes per connection
(but could be more depending on your settings ofc).
Annoyingly, Valgrind doesn't find it because the context is still
in the `EG(regular_list)` and will thus be destroyed at the end of
the request. However, I still think this bug needs to be fixed because
as the users in the issue report already mentioned:
there can be long-running PHP scripts.
Fix it by decreasing the refcount to transfer the ownership.
Closes GH-10909.
The char arrays were too small for a long on 64-bit systems, which
resulted in cutting off the string at the end with a NUL byte. Use a
size of MAX_LENGTH_OF_LONG to fix this issue instead of a fixed size
of 11 chars.
Closes GH-10525.
get_browser() implements a lazy parse system for the browscap
INI configuration. There are two possible moments when a browscap
configuration can be loaded: during module startup or during request.
In case of module startup, the strings are persistent strings, while for
the request they are not.
The INI parser must therefore know whether to create persistent or
non-persistent strings. It does this by looking at
CG(ini_parser_unbuffered_errors). If that value is 1 it's persistent,
otherwise non-persistent. Note that this also controls how the errors
are reported: if it's 1 then the errors are sent to stderr, otherwise we
get E_WARNINGs.
Currently, a hardcoded value of 1 is always used for that CG value in
browscap_read_file(). This means we'll always create persistent strings
*and* we'll not report parse errors correctly as E_WARNINGs.
We fix both the crash and the lack of warnings by passing the value of
persistent instead of a hardcoded 1.
This is also in line with how other INI parsing code is called in
ext/standard: they also make sure that during request a value of 0 is
passed.
Closes GH-10883.
This happens when there are spaces are in the path info. The reason is
that Apache decodes the path info part in the SCRIPT_NAME as per CGI
RFC. FPM tries to strip path info from the SCRIPT_NAME but the
comparison is done against SCRIPT_FILENAME which is not decoded. For
that to work we have to decode it before comparison if there is any
encoded character.
Closes GH-10869
Fixes GH-8789.
Fixes GH-10015.
This is one small part of the underlying bug for GH-10737, as in my
attempts to reproduce the issue I constantly hit this crash easily.
(The fix for the other underlying issue for that bug will follow soon.)
It's possible that a signal arrives at a thread that never handled a PHP
request before. This causes the signal globals to dereference a NULL
pointer because the TSRM pointers for the thread aren't set up to point
to the thread resources yet.
PR GH-9766 previously fixed this for master by ignoring the signal if
the thread didn't handle a PHP request yet. While this fixes the crash
bug, I think the solution is suboptimal for 3 reasons:
1) The signal is ignored and a message is printed saying there is a bug.
However, this is not a bug at all. For example in Apache, the signal
set up happens on child process creation, and the thread resource
creation happens lazily when the first request is handled by the
thread. Hence, the fact that the thread resources aren't set up yet
is not actually buggy behaviour.
2) I believe since it was believed to be buggy behaviour, that fix was
only applied to master, so 8.1 & 8.2 keep on crashing.
3) We can do better than ignoring the signal. By just acting in the
same way as if the signals aren't active. This means we need to
take the same path as if the TSRM had already shut down.
Closes GH-10861.
* Missing check: SQLAllocHandle() for the environment wasn't checked in
pdo_odbc_handle_factory(). Add a check similar to the other ones for
SQLAllocHandle().
* Inconsistent check: one of the SQLAllocHandle() calls wasn't checked
for SQL_SUCCESS_WITH_INFO. However, looking at the other uses and the
documentation we should probably check this as well.
Furthermore, since there was a mix of "SQLAllocHandle: reason" and
"SQLAllocHandle (reason)" in the error reporting, I made them
consistently use the first option as that seems to be the most used for
error reporting in this file.
Closes GH-10740.
Fixes GH-10801
Named arguments are not supported by the constant evaluation routine, in
the sense that they are ignored. This causes two issues:
- It causes a crash because not all oplines belonging to the call are
removed, which results in SEND_VA{L,R} which should've been removed.
- It causes semantic issues (demonstrated in the test case).
This case never worked anyway, leading to crashes or incorrect behaviour,
so just prevent CTE of calls with named parameters for now.
We can choose to support it later, but introducing support for this in
a stable branch seems too dangerous.
This patch does not change the removal of SEND_* opcodes in remove_call
because the crash bug can't be triggered anymore with this patch as
there are no named parameters anymore and no variadic CTE functions
exist.
Closes GH-10811.
We need to carry around a reference to the underlying Bucket to be able to modify it by reference.
Closes GH-10749
Signed-off-by: George Peter Banyard <girgias@php.net>
Disable opcache.consistency_checks.
This feature does not work right now and leads to memory leaks and other
problems. For analysis and discussion see GH-8065. In GH-10624 it was
decided to disable the feature to prevent problems for end users.
If end users which to get some consistency guarantees, they can rely on
opcache.protect_memory.
Closes GH-10798.
Fixes GH-8646
See https://github.com/php/php-src/issues/8646 for thorough discussion.
Interned strings that hold class entries can get a corresponding slot in map_ptr for the CE cache.
map_ptr works like a bump allocator: there is a counter which increases to allocate the next slot in the map.
For class name strings in non-opcache we have:
- on startup: permanent + interned
- on request: interned
For class name strings in opcache we have:
- on startup: permanent + interned
- on request: either not interned at all, which we can ignore because they won't get a CE cache entry
or they were already permanent + interned
or we get a new permanent + interned string in the opcache persistence code
Notice that the map_ptr layout always has the permanent strings first, and the request strings after.
In non-opcache, a request string may get a slot in map_ptr, and that interned request string
gets destroyed at the end of the request. The corresponding map_ptr slot can thereafter never be used again.
This causes map_ptr to keep reallocating to larger and larger sizes.
We solve it as follows:
We can check whether we had any interned request strings, which only happens in non-opcache.
If we have any, we reset map_ptr to the last permanent string.
We can't lose any permanent strings because of map_ptr's layout.
Closes GH-10783.
Due to an incorrect check, the datetime was never actually set.
To test this we need to write the file using phar, but read the file
using a different method to not get a cached, or a value that's been
transformed twice and is therefore accidentally correct.
Closes GH-10769
The docs say that this function returns true on success, and false on
error. This function always returns true in the current implementation
because the success return value from ftp_close() is never propagated to
userland. This affects one test: since the test server exits after an
invalid login, the ftp close correctly fails (because the server has
gone away).
Remove capstone include folder.
For most of the supported systems it worked fine somehow despite
the pkg-config --cflags, but is always include it even on Linux.
Closes GH-10732.
Fixes GH-10715
When a string starting with a NUL character is passed to
phpdbg_vprint(), the vasprintf() will return that 0 characters have been
printed. This causes msglen == 0. When phpdbg_process_print() is called
with a message of length 0, the -1 to check for '\n' will perform an out
of bounds read. Since nothing is printed anyway for msglen == 0, it
seems best to just skip the printing routine for this case.
Closes GH-10720.
SSL_CTX_set_tmp_dh() and SSL_CTX_set0_tmp_dh_pkey() return 1 on success
and 0 on error. But only < 0 was checked which means that errors were
never caught.
Closes GH-10705.
Fixes GH-10692
php_fopen_primary_script() does not initialize all fields of
zend_file_handle. So when it fails and when fastcgi is true, the
zend_destroy_file_handle() function will try to free uninitialized
pointers, causing a segmentation fault. Fix it by zero-initializing file
handles just like the zend_stream_init_fp() counterpart does.
Closes GH-10697.
zend_update_static_property_ex() returns a zend_result, but the return
value is stored here in a bool. A bool is unsigned on my system, so in
case zend_update_static_property_ex() returns FAILURE (== -1) this gets
converted to 1 instead. This is not a valid zend_result value. This
means that (transitive) callers could mistakingly think the function
succeeded while it did in fact not succeed. Fix it by changing the type
to zend_result.
Closes GH-10691.
The length of "output_handler" is supposed to be passed, but as sizeof
is used, the resulting number includes the NUL character, so the length
is off-by-one. Subtract one to pass the correct length.
Closes GH-10667.
In SessionHandler::gc, we use a virtual call to PS(default_mod)->s_gc to
call the gc implementation. That return value is checked against
FAILURE (-1).
One of the call targets of PS(default_mod)->s_gc is ps_gc_files().
ps_gc_files() calls to ps_files_cleanup_dir(). The latter function has
some error checks and outputs a notice if something goes wrong. In cases
of errors, the function returns 0. This means that the check in
SessionHandler::gc will misinterpret this as a success and report that 0
files have been *successfully* cleaned up. Fix it by returning -1 to
indicate something *did* go wrong.
Closes GH-10644.
pcre2_match() returns error codes < 0, but only the "no match" error
code was handled. Fix it by changing the check to >= 0.
Closes GH-10632
Signed-off-by: George Peter Banyard <girgias@php.net>
Parse errors were not reported for the default config, they were only
reported when explicitly another config was loaded.
This means that users may not be aware of errors in their configuration
and therefore the behaviour of Tidy might not be what they intended.
This patch fixes that issue by using a common function. In fact, the
check for -1 might be enough for the current implementation of Tidy, but
the Tidy docs say that any value other than 0 indicates an error.
So future errors might not be caught when just using an error code of -1.
Therefore, this also changes the error code checks of == -1 to < 0 and
== 1 to > 0.
Closes GH-10636
Signed-off-by: George Peter Banyard <girgias@php.net>
Fixes GH-10627
The php_mb_convert_encoding() function can return NULL on error, but
this case was not handled, which led to a NULL pointer dereference and
hence a crash.
Closes GH-10628
Signed-off-by: George Peter Banyard <girgias@php.net>
