This was first pointed out in GH-10959.
The from_zval_... functions don't always write to the pointer, in particular
it is necessary to check for an error before using the value. Otherwise
we can access an uninitialized value and that's UB (and dangerous).
Note: this does *NOT* get rid of the compiler warning. Even though there
is error checking now, the compiler isn't smart enough to figure out
that the values can not be used uninitialized.
Closes GH-10966.
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.
Signed multiply overflow is undefined behaviour.
If you run the CI tests with UBSAN enabled on a 32-bit platform, this is
quite easy to hit. On 64-bit it's more difficult to hit though, but not
impossible.
Closes GH-10942.
As shown on the CI runs on my fork (which runs with UBSAN),
the pointers can sometimes be unaligned when trying to write.
This is UB and on platforms like ARM this *can* result in a bus error.
Replace it with memcpy, which at least on x86 and powerpc
architectures does result in the same assembly code.
Closes GH-10940.
At least on 32-bit, the address computations overflow in running the
test on CI with UBSAN enabled. Fix it by reordering the arithmetic.
Since a part of the expression is already used in the code above the
computation, this should not negatively affect performance.
Closes GH-10936.
It's possible that curl was compiled without SSL, and/or without libz
support. In the case of the issue reporter it was without libz support.
This causes the test to fail because we expect a non-empty string.
Fix it by using %S instead of %s to allow empty strings.
Closes GH-10930.
Previously, mbstring used the same logic for encoding validation as for
encoding conversion.
However, there are cases where we want to use different logic for validation
and conversion. For example, if a string ends up with missing input
required by the encoding, or if a character is input that is invalid
as an encoding but can be converted, the conversion should succeed and
the validation should fail.
To achieve this, a function pointer mb_check_fn has been added to
struct mbfl_encoding to implement the logic used for validation.
Also, added implementation of validation logic for UTF-7, UTF7-IMAP,
ISO-2022-JP and JIS.
(The same change has already been made to PHP 8.2 and 8.3; see
6fc8d014df. This commit is backporting the change to PHP 8.1.)
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.
Commit a21195650e fixed a leak by adding a TSRM destructor for the
JIT globals in ZTS mode. In case the main thread shuts down the TSRM, it
will call all the destructors. The JIT globals destructor will be
invoked, but will always access the main thread globals using JIT_G.
This means that instead of freeing the JIT globals in the different
threads, the one in the main thread is freed repeatedly over and over,
crashing PHP. Fix it by always passing the pointer instead of relying on
JIT_G.
Closes GH-10835.
