* Fix a double free when zipping keys and scores.
* Instead of aborting with an assertion if elements != 2 just warn and
return failure
* Instead of crashing on `xclaim` reply shape issues, just return false
* Switch to using `zend_string*` in a lot of places that were previously
deconstructing the `zend_string` into the char and len.
* Various other minor bits of cleanup.
Rework `cluster_session_key` to return either a newly allocated
`zend_string*` when we have a prefix, or a cheap copy when we don't.
Previously we were using `int` to curry the length which was in theory
susceptible to overflow if `ZSTR_LEN(prefix) + keylen > INT_MAX)`.
Fixes#2866
Follow-up to the initial array_init_size pass. Converts the remaining
reply-array builders in library.c that have an element count available
at construction time, so multi-element replies don't pay HashTable
resizes as they fill: stream replies (XRANGE/XREAD/XINFO/XCLAIM/
XAUTOCLAIM), ACL LOG/GETUSER/CAT, GEOSEARCH, FUNCTION LIST, COMMAND
INFO, vlinks, the recursive variant reader, and pub/sub unsubscribe.
INFO, CLIENT INFO and CLIENT LIST are sized from a cheap delimiter
pre-count before tokenizing. Two known-empty results use
ZVAL_EMPTY_ARRAY (no HashTable allocation). redis_xrange_reply now
reads its message count before initializing the result so it can be
sized too.
Counts are clamped to >= 0 (a null multibulk yields -1); array_init_size
with a count <= 8 is identical to array_init, so the few fixed-small
sites are converted only for consistency.
Verified on PHP 8.4: streams, ACL, geo, function/command, INFO/CLIENT
parsing, ZMPOP/LMPOP and unsubscribe round-trip correctly; no leaks
under report_memleaks; testInfo/testClient/testZMPop/testLMPop/
testXAutoClaim/testSubscribe pass.
redis_mbulk_reply_loop read each element with redis_sock_read, which
emalloc's the bulk body into a raw char*, then copied it into a
zend_string via ZVAL_STRINGL and freed the char*. With no serializer or
compression active, the unwrap path through redis_unpack also resolves
to a plain copy, so every element of MGET/LRANGE/SMEMBERS/HGETALL paid
two allocations and a full-payload memcpy.
Add redis_sock_read_zstr, a zend_string-returning counterpart to
redis_sock_read, and redis_sock_read_bulk_zstr, which materializes the
bulk body straight into a zend_string. The loop moves that string into
the result array with ZVAL_STR (no copy) and only calls redis_unpack
when a serializer or compression is actually configured, in which case
it passes the string buffer and releases it afterward.
Verified on PHP 8.4: zero-copy and serializer (repack) paths, bulk edge
cases (empty, binary with NUL/CRLF, 1MB, null/missing), per-element
key/value parity for HGETALL, no leaks under report_memleaks, and the
testKeys/testHashes/testLSet/testSortAsc/testZRange suite methods.
* Introduce a new `RedisCmd` struct to dynamically append RESP arguments
such that we don't have to precalculate the number of arguments the
command will have up front.
Additionally the new `RedisCmd allows both a `void *` context pointer
but also can attach a `void (*ctx_dtor)(void*)` destructor so we are
still able to clean up any allocated context when commands fail.
This moves the context cleanup out of every individual reply handler
and into the generic processing wrappers.
* Create a small group of `resp_str` helper functions for lower level
concatination of RESP protocol data over the wire.
* Lots of small modernization of the codebase such as using
`zend_string*` instead of (`char *`, `size_t`) pairs.
* Greatly simplify `crosslot` handling logic
redis_sock_read_bulk_reply called redis_check_eof(), which issues a
php_stream_eof() probe (a recv(MSG_PEEK) syscall when the stream buffer
is drained), before reading the bulk body. Every caller reaches this
function only after successfully reading the bulk-length header on the
same socket, which already proved the stream live, so the probe is
redundant and adds a kernel round-trip to the dominant GET/HGET/MGET
read path.
Replace it with a cheap NULL-stream guard. A disconnect that happens
mid-read is still caught by the existing php_stream_eof() check inside
the read loop.
cluster_dist_write emalloc'd a small int array (master + slaves) on every
read command issued under a failover-distribution mode, then freed it.
A shard's replica count is small in practice, so build the index list on
a 16-element stack array and only fall back to emalloc for larger
fan-outs. Both free sites are now guarded against freeing the stack.
When a RedisArray uses a custom hash algorithm, ra_find_node allocated
the hash context and digest buffers on the heap for every key lookup.
Both are small (the largest common context is SHA-512 at ~208 bytes),
so use stack buffers for the common case and fall back to emalloc only
when an algorithm's context or digest exceeds them.
The context buffer is a union with a double member to guarantee the
alignment the context structs require for their uint64_t state.
Several multibulk reply builders called array_init (8-bucket default)
right after reading the element count off the wire, forcing one or more
HashTable resizes as elements were appended. Switch these sites to
array_init_size using the known count.
The count is clamped to >= 0 because a null multibulk header yields -1,
and array_init_size takes a uint32_t; a negative value would otherwise
request a huge table. array_init_size(_, 0) is equivalent to array_init,
so the clamp is never worse than the prior behavior.
Covers redis_sock_read_multibulk_reply_zval, the LPOS COUNT path,
CLIENT TRACKINGINFO, HELLO, and nested multibulk in the recursive
variant reader.
cluster_send_command captured msstart via mstime() (a gettimeofday call)
on entry to every command, but the elapsed-time check below already only
calls mstime() when c->waitms is non-zero. When no request timeout is
configured, the initial capture is dead work. Gate it on c->waitms too.
redis_key_prefix used ecalloc to allocate the prefixed-key buffer, then
immediately overwrote the entire allocation with two memcpy calls. The
zero-fill was wasted work on every keyed argument when a prefix is set.
Use emalloc and write the single trailing NUL explicitly.
Clamp range in a couple library functions to values that can fit into an
iint, since we narrow it later.
A more comprehensive change that widens all of these values to 64 bits
will come in a future commit.
1. Make sure slot ranges are in bounds and that `high >= low`
2. Make sure any returned host lens are not >= `sizeof(c->redir_host)` so
they can be stored and null terminated.
3. Make sure all returned ports are sane (0-65535).
Previously a corrupted or malicious `MOVED` response could embed a host
name that was larger than the `c->redir_host` buffer which could leave
it non null-terminated.
Worse, `c->redir_host_len` was calculated from the too-large input which
could cause subsequent use to memcpy past the end of our buffer.
This fix simply hard rejects any host that we can't store in
`c->redir_host` while including a null terminator.
In addition we swich from a statically sized buffer in
`RedisCluster::_redir` to using `zend_smart_str`
Previously we were only checking if `LZ4_decompress_safe` was returning
> 0 but then blindly returning to the user whatever length the header
specified.
This fix does two things:
* Short circuits on negative length headers
* Fails the decompression if the decompressed length does not match.
Three independent hardening fixes against malicious cluster replies,
shipped together because they share the same threat model and live
within a few lines of each other in cluster_library.c.
Reject nil-bulk hosts in CLUSTER SLOTS rows. VALIDATE_SLOTS_INNER
checked type == TYPE_BULK but not str != NULL || len > 0, so a
hostile seed could drive redis_sock_create(NULL, (size_t)-1) into
zend_string_init's memmove from NULL. Apply the same str/len gate to
the slaves loop, which previously skipped only on len == 0.
Store ASK-redirect nodes in c->nodes. cluster_get_asking_node built
a fresh redisClusterNode on every ASK to an unknown host but never
inserted it, leaking the node plus its RedisSock, AUTH zend_strings,
slaves HashTable, and persistent connection for the cluster object's
lifetime.
Bound the redirect port and slot. atoi-into-(unsigned short) let a
hostile MOVED / ASK target reach port mod 65536; replace with strtol
plus explicit range checks and reject out-of-range values.
php_stream_getc returns int; storing into a char truncates the EOF
sentinel. On unsigned-char platforms (ARM Linux, AIX, PPC) EOF == -1
becomes 0xFF after promotion and res != EOF is always true; on
signed-char platforms a server byte of 0xFF is indistinguishable
from real EOF. Subscribe loops can busy-loop or stall. Return int,
matching the standard getc-style convention.
atol returns undefined behavior on overflow per C11 7.22.1.4. glibc
saturates to LONG_MAX, but musl, BSD libc, and Windows libc differ.
Replace atol / atoi at the three RESP length parse sites in library.c
with strtoll plus ERANGE rejection. The wire input is server-
controlled; an out-of-range value should drop the reply rather than
land an implementation-defined value in downstream length arithmetic.
generate_lock_secret derived the secret from hostname plus pid,
roughly 22 bits of guessable entropy and also readable from Redis
under <key>_LOCK. Replace with 16 bytes from php_random_bytes_silent,
hex-encoded.
Defense in depth: an attacker with write access to the Redis
instance can already bypass the lock by DELing the key, so this is
not a primary defense; worth fixing for the case where only the
lock key itself is exposed. The hostname|pid path stays as a fallback
when php_random_bytes_silent fails, so the caller always gets a
non-NULL secret.
Rarely CI hangs forever (or until it hits the maximum workflow execution
time) when Redis doesn't come up properly.
This just adds a configurable timeout so we can rerun the workflow when
it hangs.
Add support for Redis' `DELEX` and Valkey's `DELIFEQ` when deleting the
session lock key. Local testing shows about a 10-15% improvement over
the current `EVAL[SHA]` strategy.
This commit adds a new INI settingg:
```ini
redis.session.lock_release_cmd = delex|delifeq|eval
```
By default we continue to use the `EVAL` logic and if a user specifies
another mechanism but the command doesn't exist, we warn the user and
fall back to EVAL.
This commit also refactors a few functions to avoid UB and simplify key
construction.
The stub declares $seeds as ?array but the C code used format
specifier 'a' (non-nullable) instead of 'a!' in
zend_parse_method_parameters. This caused new RedisCluster(null, null)
to throw TypeError instead of RedisClusterException, contradicting
the declared type signature.
Also treat z_seeds == NULL the same as ZEND_NUM_ARGS() < 2 so that
explicitly passing null falls through to INI-based seed loading,
matching the behaviour when the argument is omitted entirely.
Fixes GH-2810.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Instead of currying around a `php_stream_context` object, just retain
the context array provided by the user itself like we do with other
connection information like host and port. This lets users reconnect in
a loop without leaking memory.
```php
$redis = new \Redis;
while (true) {
// Previously each reconnect call would leak the
// `php_stream_context` structure.
$redis->connect('tls://127.0.0.1', 9999, 1, null, 0, 0, [
'stream' => ['verify_peer' => false, 'verify_peer_name' => false],
]);
$redis->ping();
$redis->close();
}
```
michael-grunder