Skip to content

New command flag to indicate modification of first key only - #8

Merged
JimB123 merged 1 commit into
jims-forklessfrom
f-firstkey
Jan 8, 2026
Merged

JimB123 merged 1 commit into
jims-forklessfrom
f-firstkey

Conversation

@JimB123

@JimB123 JimB123 commented Jan 8, 2026

Copy link
Copy Markdown
Owner

No description provided.

Signed-off-by: Jim Brunner <brunnerj@amazon.com>
@JimB123
JimB123 merged commit 24346be into jims-forkless Jan 8, 2026
43 checks passed
JimB123 pushed a commit that referenced this pull request Feb 18, 2026
…lkey-io#3174)

I was working on ASAN large memory tests when I countered this issue.

The issue was that the hardcoded `999` key could land in an early
bucket. Then shrink rehash could finish early, and later inserts could
trigger a new expansion rehash, resetting rehash_idx low. The test now
picks the survivor key dynamically as the key mapped to the highest
bucket index.

```
[test_hashtable.c] Memory leak detected of 336 bytes
=================================================================
==3901==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 80 byte(s) in 1 object(s) allocated from:
    #0 0x7fb0556fd9c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x563bfdf4c47d in ztrymalloc_usable_internal /home/runner/work/valkey/valkey/src/zmalloc.c:156
    #2 0x563bfdf4c47d in valkey_malloc /home/runner/work/valkey/valkey/src/zmalloc.c:185
    #3 0x563bfdd42eaf in hashtableCreate /home/runner/work/valkey/valkey/src/hashtable.c:1217
    #4 0x563bfdaa1cbf in test_empty_buckets_rehashing unit/test_hashtable.c:232
    #5 0x563bfdae772b in runTestSuite unit/test_main.c:36
    #6 0x563bfda86b20 in main unit/test_main.c:108
    #7 0x7fb05522a1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #8 0x7fb05522a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #9 0x563bfda8a5c4 in _start (/home/runner/work/valkey/valkey/src/valkey-unit-tests+0x17c5c4) (BuildId: 44cfc183e6e82e499bcc9f6adc094d7f774ee9d2)

Indirect leak of 128 byte(s) in 1 object(s) allocated from:
    #0 0x7fb0556fd340 in calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
    #1 0x563bfdf4c922 in ztrycalloc_usable_internal /home/runner/work/valkey/valkey/src/zmalloc.c:214
    #2 0x563bfdf4c922 in valkey_calloc /home/runner/work/valkey/valkey/src/zmalloc.c:257
    #3 0x563bfdd40967 in resize /home/runner/work/valkey/valkey/src/hashtable.c:741
    #4 0x563bfdd45eb1 in hashtableExpandIfNeeded /home/runner/work/valkey/valkey/src/hashtable.c:1446
    #5 0x563bfdd45eb1 in hashtableExpandIfNeeded /home/runner/work/valkey/valkey/src/hashtable.c:1433
    #6 0x563bfdd45eb1 in insert /home/runner/work/valkey/valkey/src/hashtable.c:1041
    #7 0x563bfdd45eb1 in hashtableAddOrFind /home/runner/work/valkey/valkey/src/hashtable.c:1554
    #8 0x563bfdd45eb1 in hashtableAdd /home/runner/work/valkey/valkey/src/hashtable.c:1539
    #9 0x563bfdaa1e3b in test_empty_buckets_rehashing unit/test_hashtable.c:254
    #10 0x563bfdae772b in runTestSuite unit/test_main.c:36
    #11 0x563bfda86b20 in main unit/test_main.c:108
    #12 0x7fb05522a1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #13 0x7fb05522a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #14 0x563bfda8a5c4 in _start (/home/runner/work/valkey/valkey/src/valkey-unit-tests+0x17c5c4) (BuildId: 44cfc183e6e82e499bcc9f6adc094d7f774ee9d2)

Indirect leak of 64 byte(s) in 1 object(s) allocated from:
    #0 0x7fb0556fd340 in calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
    #1 0x563bfdf4c922 in ztrycalloc_usable_internal /home/runner/work/valkey/valkey/src/zmalloc.c:214
    #2 0x563bfdf4c922 in valkey_calloc /home/runner/work/valkey/valkey/src/zmalloc.c:257
    #3 0x563bfdd3f553 in bucketConvertToChained /home/runner/work/valkey/valkey/src/hashtable.c:908
    #4 0x563bfdd3f553 in findBucketForInsert /home/runner/work/valkey/valkey/src/hashtable.c:1021
    #5 0x563bfdd45d9e in insert /home/runner/work/valkey/valkey/src/hashtable.c:1045
    #6 0x563bfdd45d9e in hashtableAddOrFind /home/runner/work/valkey/valkey/src/hashtable.c:1554
    #7 0x563bfdd45d9e in hashtableAdd /home/runner/work/valkey/valkey/src/hashtable.c:1539
    #8 0x563bfdaa1e3b in test_empty_buckets_rehashing unit/test_hashtable.c:254
    #9 0x563bfdae772b in runTestSuite unit/test_main.c:36
    #10 0x563bfda86b20 in main unit/test_main.c:108
    #11 0x7fb05522a1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #12 0x7fb05522a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #13 0x563bfda8a5c4 in _start (/home/runner/work/valkey/valkey/src/valkey-unit-tests+0x17c5c4) (BuildId: 44cfc183e6e82e499bcc9f6adc094d7f774ee9d2)

Indirect leak of 64 byte(s) in 1 object(s) allocated from:
    #0 0x7fb0556fd340 in calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
    #1 0x563bfdf4c922 in ztrycalloc_usable_internal /home/runner/work/valkey/valkey/src/zmalloc.c:214
    #2 0x563bfdf4c922 in valkey_calloc /home/runner/work/valkey/valkey/src/zmalloc.c:257
    #3 0x563bfdd40967 in resize /home/runner/work/valkey/valkey/src/hashtable.c:741
    #4 0x563bfdaa1df8 in test_empty_buckets_rehashing unit/test_hashtable.c:248
    #5 0x563bfdae772b in runTestSuite unit/test_main.c:36
    #6 0x563bfda86b20 in main unit/test_main.c:108
    #7 0x7fb05522a1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #8 0x7fb05522a28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #9 0x563bfda8a5c4 in _start (/home/runner/work/valkey/valkey/src/valkey-unit-tests+0x17c5c4) (BuildId: 44cfc183e6e82e499bcc9f6adc094d7f774ee9d2)

SUMMARY: AddressSanitizer: 336 byte(s) leaked in 4 allocation(s).
```

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
JimB123 pushed a commit that referenced this pull request Aug 10, 2026
)

### Problem

The bug was originally introduced by valkey-io#1737

In tls.c, we normally avoid consolidating IO vectors into a buffer if
they exceed a capped amount (`NET_MAX_WRITES_PER_EVENT`, which is 64KB).
However, on OpenSSL write errors, we must never trigger a subsequent
write that is smaller than the previous failed write. In this path, we
must consolidate the IO vectors to ensure the write is at least as large
as the last write we made on the socket:

```c
    char buf[iov_bytes_len];
    size_t offset = 0;
    for (int i = 0; i < iovcnt && offset < iov_bytes_len; i++) {
        memcpy(buf + offset, iov[i].iov_base, iov[i].iov_len);
```

However, `iov_bytes_len` can easily exceed the stack boundaries if one
of the IO vectors is large (e.g. a large key read), leading to a crash:

```
Thread 1 (Thread 0x7ffff7b65700 (LWP 191) "valkey-server"):
#0  0x00005555557a04bc in connTLSWritev (conn_=0x7ffff6e518c0, iov=<optimized out>, iovcnt=5) at src/tls.c:1713
#1  0x00005555556d2a21 in connWritev (conn=<optimized out>, iov=0x7fffffff9240, iovcnt=5) at src/connection.h:256
#2  writevToClient (c=<optimized out>) at src/networking.c:2814
#3  _writeToClient (c=<optimized out>) at src/networking.c:2868
#4  0x00005555556d326c in writeToClient (c=0x7ffff6e7a780) at src/networking.c:3117
#5  writeToClient (c=0x7ffff6e7a780) at src/networking.c:3108
#6  sendReplyToClient (conn=<optimized out>) at src/networking.c:3127
#7  0x00005555557a0ffd in callHandler (conn=0x7ffff6e518c0, handler=<optimized out>) at src/connhelpers.h:79
#8  tlsHandleEvent (conn=0x7ffff6e518c0, mask=<optimized out>) at src/tls.c:1507
#9  0x00005555555fbad5 in aeProcessEvents (flags=27, eventLoop=0x7ffff6e45f80) at src/ae.c:504
#10 aeMain (eventLoop=0x7ffff6e45f80) at src/ae.c:543
#11 0x00005555555eb83a in main (argc=2, argv=0x7fffffffd588) at src/server.c:7833
```

### Fix

To fix this, we simply avoid allocating large buffers on the stack by
using a fixed-size stack buffer `char buf[NET_MAX_WRITES_PER_EVENT]`
(64KB) and performing partial copies:
1. Calculate total length of all `iov` blocks to decide if we can use
one-by-one writing.
2. If total length is larger than `NET_MAX_WRITES_PER_EVENT` (64KB) and
`iov[0]` is large enough to satisfy `last_failed_write_data_len`, write
blocks sequentially one-by-one.
3. Otherwise (combine path), copy data from `iov` into a fixed-size
stack buffer of `NET_MAX_WRITES_PER_EVENT` (64KB), capping the copy at
64KB (partial copy if it exceeds).
4. Verify that we copied at least `last_failed_write_data_len` to
satisfy OpenSSL retry constraint.
5. Write the combined buffer and return the number of bytes copied
(Valkey connection layer will handle the remaining data as a partial
write retry).

Important: this should keep the existing "write must be larger than last
failed write" invariant, considering: 1) if the last write went to the
consolidated buffer path, it would be at most `NET_MAX_WRITES_PER_EVENT`
2) if the last write did not go through the consolidated buffer path, it
will be at the front of the `iov`, and therefore our `iov[0] >=
last_failed_write_data_len` check will always pass (it is the same
write).

Finally, since we added IO threaded TLS writes, the stack limit may be
even smaller for the thread (depending on the system). We work around
this in bio threads, and I am copying that workaround to IO threads to
ensure the stack is not smaller than `NET_MAX_WRITES_PER_EVENT`.

### Testing

A regression test was appended to the existing TLS test suite in
tests/unit/tls.tcl. We need to add a new debug subcommand `DEBUG
FORCE-TLS-WRITE-ERROR <0 or 1>` to deterministically trigger OpenSSL
write failures. With this flow, we write a command with a small response
and force it to trigger an IO error, then we write a command with a
larger response and resolve the IO errors. The response to the large
command will accumulate in the buffer, and we will attempt to write out
the full output buffer for the client, which now includes a massive
final IO vector. In the old code, this would trigger a stack overflow as
we try to allocate O(megabytes) on the stack.

---------

Signed-off-by: Jacob Murphy <jkmurphy@google.com>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant