Fix integer overflow in RESP output buffer growth (HGETALL on large hashes)

[hexonal]

Fixes #1616

Root cause

RespMemoryWriter.ReallocateOutput (the buffer-growth routine backing every Write* method, including the ones HashGetAll uses for HGETALL) computed its next buffer capacity with int/uint arithmetic and only special-cased an exact match on 0x40000000 (2^30) to avoid overflow:

if (length == 0x40000000)
    length = Array.MaxLength;
else if (length < extraLenHint)
{
    var total = (uint)extraLenHint + (uint)length;
    ...
    length = (int)BitOperations.RoundUpToPowerOf2(total);
}
else
    length <<= 1;

Once the output buffer's capacity grows large enough (which happens for HGETALL on hashes with enough elements/data that the response requires many buffer doublings) and its size doesn't happen to land exactly on 0x40000000 on the way up — which is the common case, since the buffer's starting size isn't guaranteed to be a power of two — length <<= 1 can overflow a 32-bit signed int and wrap around to a small or negative value. The subsequent Buffer.MemoryCopy(ptr, newPtr, length, bytesWritten) then receives a length smaller than bytesWritten, which throws ArgumentOutOfRangeException for the sourceBytesToCopy parameter — exactly the crash reported in the issue. (Other wrap patterns in the same code hit an OverflowException("length") instead; both are opaque, unhandled-by-design exceptions that kill the client connection with no response at all, since RespServerSession.ProcessMessages's generic catch (Exception) handler doesn't send anything back to the client.)

Fix

• Extracted the growth calculation into RespMemoryWriter.ComputeGrowth(currentLength, extraLenHint, lowerMinimum), doing all arithmetic in long/ulong so it can no longer silently wrap around into a value smaller than what's required. The result is clamped to Array.MaxLength, the true ceiling for a single managed array / MemoryPool<byte>.Shared.Rent buffer.
• ReallocateOutput now checks that the computed length can actually hold what's already been written and that real growth occurred (guarding the edge case where the buffer is already at Array.MaxLength and can't grow further, which would otherwise spin forever re-renting an identically-sized buffer). If the response genuinely can't fit in one buffer, it raises a GarnetException with a clear, actionable message pointing at cursor-based alternatives (e.g. HSCAN instead of HGETALL) — this is returned to the client as a normal RESP error instead of crashing the connection.

This is a general fix to the shared RESP output-buffer growth path (used by every large-response command, not just HGETALL specifically), but it directly addresses the HGETALL crash reported in #1616.

Testing performed

• Added RespMemoryWriterTests (test/standalone/Garnet.test/Resp/RespMemoryWriterTests.cs), unit tests exercising ComputeGrowth at the exact boundary values the old arithmetic mishandled (0x40000000, 0x40000001, 0x60000000, values near Array.MaxLength, and a full repeated-doubling walk from a non-power-of-two starting size up to the ceiling). I independently reproduced the old formula in a scratch project and confirmed these specific inputs throw under the old logic and pass under the new one.
• Added CanDoHGETALLOnLargeHashRequiringManyBufferGrowths (test/standalone/Garnet.test.collections/RespHashTests.cs), an end-to-end test against a real Garnet server with 50,000 hash fields that forces many real buffer reallocations through the actual write path, and verifies HGETALL returns the complete, correct data.
• Ran the full existing RespHashTests suite (95 tests incl. the new one) and the Garnet.test Resp-namespace suite (752 tests, 2 pre-existing/unrelated skips) — all pass.
• I did not reproduce the original report's exact workload (a multi-million-element hash whose HGETALL response reaches multi-GB serialized size) end-to-end — that scale is impractical to run in CI or locally. The fix is verified at the unit level against the precise arithmetic boundaries that caused the crash, plus a smaller-scale integration test exercising the same code path.

[Copilot]

Pull request overview

This PR addresses a crash when generating very large RESP responses (reported via HGETALL on multi-million-element hashes) by fixing integer overflow/wraparound in RespMemoryWriter’s output-buffer growth logic. The fix applies to the shared RESP writing path used by many large-response commands, and adds both unit and integration regression coverage.

Changes:

• Refactors output buffer growth into RespMemoryWriter.ComputeGrowth(...) using long arithmetic and clamping to Array.MaxLength, and adds explicit failure behavior when growth is impossible.
• Adds unit tests for boundary/overflow scenarios around 2³⁰–2³¹ and Array.MaxLength.
• Adds an end-to-end regression test that forces many reallocations and validates HGETALL correctness on a large hash.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
libs/common/RespMemoryWriter.cs	Reworks buffer growth calculation to avoid overflow and throws a clearer error when a single-buffer response cannot be produced.
test/standalone/Garnet.test/Resp/RespMemoryWriterTests.cs	Adds targeted unit tests for buffer growth boundary conditions and monotonicity.
test/standalone/Garnet.test.collections/RespHashTests.cs	Adds an integration regression test that exercises the real RESP write path via HGETALL on a large hash.

[hexonal]

Good catch, fixed in df7cf71 — this error is a client-visible, request-level condition (this one response exceeds Array.MaxLength), not server-state corruption, so there's no real reason to kill the connection.

Checked the actual throw site: it happens before any buffer is rented or pointer reassigned in ReallocateOutput, and the RespMemoryWriter for the failing command is always used in a using block (see the call sites in HashObjectImpl.cs etc.), so nothing shared across commands on the connection is left inconsistent when the exception unwinds. Added disposeSession: false and a comment explaining why it's safe.

For test coverage: the general "disposeSession: false keeps the connection usable for the next command" mechanism (the part of RespServerSession.cs this change actually exercises) is already covered end-to-end by the existing GarnetObjectStoreDisabledError test, which hits the identical code path via a different GarnetException call site. I didn't add a new end-to-end repro specifically for this trigger, since reaching it for real requires an actual response near Array.MaxLength (~2GB) — impractical for a fast unit test and consistent with this PR's own disclosed limitation that the original multi-GB-scale repro wasn't run end-to-end either. Let me know if you'd want that covered some other way (e.g. a seam to inject a smaller ceiling for testing).

[hexonal]

Pushed a follow-up commit fixing a dotnet format violation in the new test file (comment indentation + missing trailing newline) that was failing the "Format Garnet" check.

For visibility on the other 3 red checks from the previous run — none touch the code this PR changes:

• Garnet Standalone (ubuntu-latest, net8.0, Release, Garnet.test): failed on RespAdminCommandsTests.SeSaveRecoverMultipleKeysTest (a SAVE/RECOVER persistence assertion), unrelated to RespMemoryWriter/HGETALL.
• Both Garnet Cluster (windows-latest, .../Garnet.test.cluster.migrate) failures: ClusterTLSMT.ClusterTLSInitialize failed with a TLS transport-connection/auth error establishing the test cluster — looks like Windows-runner networking flakiness, not a code issue.

Will keep an eye on the new run in case any of these are non-flaky.

[hexonal]

CI on the latest commit (df7cf71) has 2 unrelated failures, both outside this PR's diff (which only touches libs/common/RespMemoryWriter.cs):

• RespAdminCommandsTests.SeSaveRecoverMultipleKeysTest("25k","18k") — the pre-existing, previously-reported recovery-under-memory-pressure flake tracked in Data loss on recovery: SeSaveRecoverMultipleKeysTest("25k","18k") intermittently loses a key (~50% repro rate) #1950.
• ClusterMigrateTests(False).ClusterMigrateDataSlotsRange — cluster slot-migration test, unrelated subsystem to this diff.

Both showed exactly 1 failure out of their respective suites (983 and 52 tests), everything else green.

[kevin-montrose]

CanDoHGETALLOnLargeHashRequiringManyBufferGrowths doesn't fail on main - it's not testing the actual issue.

It looks like this change would be sufficient:

--- a/libs/common/RespMemoryWriter.cs
+++ b/libs/common/RespMemoryWriter.cs
@@ -507,7 +507,7 @@ namespace Garnet.common
             }

             if (length <= 0)
-                throw new OverflowException("length");
+                throw new GarnetException(<some error message>, disposeSession: false);

             var newMem = MemoryPool<byte>.Shared.Rent(length);
             var newPtrHandle = newMem.Memory.Pin();

And add a test that overflows - back of the envelope, with 20K value length, you'd need 250K-ish hash members. Something like:

[Test]
public async Task HGETALLMemoryOverflowAsync()
{
    const string Key = nameof(HGETALLMemoryOverflowAsync);
    const int NumFields = 250_000;
    const int FieldLength = 20_000;

    using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
    var db = redis.GetDatabase();

    var fieldValue = new string('v', FieldLength); // pad each value so total payload is sizeable

    var writeTasks = new Task<bool>[NumFields];

    for (var i = 0; i < NumFields; i++)
    {
        writeTasks[i] = db.HashSetAsync(Key, $"field:{i}", fieldValue);
    }

    var writeReses = await Task.WhenAll(writeTasks).ConfigureAwait(false);
    ClassicAssert.IsTrue(writeReses.All(static x => x));

    var x = await db.HashGetAllAsync(Key).ConfigureAwait(false);

    var exc = ClassicAssert.ThrowsAsync<RedisServerException>(() => db.HashGetAllAsync(Key));
    ClassicAssert.AreEqual(<error message>, exc.Message);
}

[hexonal]

You're right that CanDoHGETALLOnLargeHashRequiringManyBufferGrowths doesn't fail on main - I've confirmed that independently. At the scale that test uses (50k fields, ~5MB total payload) the response never gets anywhere near the buffer capacities (~2^30-2^31 bytes) where the old growth arithmetic actually broke down, so it was never exercising the bug. I've corrected its doc comment so it doesn't overclaim that anymore.

On the proposed fix: I did the arithmetic out fully rather than just eyeballing it. Two things came out of that which I think are worth sharing:

1. Your minimal fix is correct and sufficient for the failure mode it targets. I proved (and cross-checked with a brute-force sweep over the critical boundary values) that in the old int/uint arithmetic, whenever the computed length comes out positive, it's always strictly greater than the buffer's prior capacity - which itself always bounds bytesWritten, since every TryWrite* helper checks end - curr before writing. So whenever growth would be insufficient, length is always <= 0, which the existing guard already catches - it just throws the wrong kind of exception (unhandled OverflowException, landing in the generic catch (Exception) and killing the connection) instead of a graceful GarnetException. Your one-line change fixes that correctly. Our current ComputeGrowth-based fix (already using disposeSession: false since Copilot's review) closes the same gap, just via long arithmetic instead of relying on that int-overflow-always-goes-negative property staying true forever.

2. I still can't account for the exact exception in the original report. The proof above means a Buffer.MemoryCopy call with a positive-but-insufficient length shouldn't be reachable through this arithmetic for any legitimate (non-negative) extraLenHint - but that's precisely what the issue's stack trace shows (ArgumentOutOfRangeException on sourceBytesToCopy, not OverflowException). I looked for another path to it (the RESP2 map-length doubling, DecreaseArrayLength's separate MemoryCopy call site, the bounds-checks in RespWriteUtils) and came up empty - everything else correctly checks capacity before writing. I don't have a full mechanistic explanation for the original crash, and I'd rather say that plainly than paper over it. My best guess is it's the same general "response outgrew what a single buffer can hold" family your test and ours both target - just observed through a different exact exception than either of us has reproduced - but that's a guess, not something I've confirmed.

Given (1), I don't think the fix itself needs to change. For test coverage, instead of the ~5GB end-to-end test, I added a fast unit test directly against ComputeGrowth (pushed in the latest commit) that asserts once the buffer is already at Array.MaxLength, growth reports "no further growth possible" - the exact signal ReallocateOutput uses to raise the client-visible error instead of looping or overflowing. I confirmed it actually catches a regression (reverting the Array.MaxLength clamp makes it fail with a negative computed length). I also added a companion test case for when both the current capacity and the incoming item are large simultaneously. I held off on your proposed integration test since the unit test exercises the identical guard condition at effectively zero CI cost, but happy to add it too if you'd still like the end-to-end coverage.