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

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.

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.

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);
}

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.

Sounds like a yes on the end-to-end coverage then -- pushed a test using your numbers (HGETALLOnHashExceedingMaxSingleBufferSizeThrowsCleanlyAsync, 250K x 20K fields).

I kept the current fix's shape (the ComputeGrowth rewrite) rather than switching to your minimal diff, since it's already in and its own boundary-value unit tests cover the arithmetic directly -- but the test is exactly your scenario and assertion shape: HashSetAsync per field, then Assert.ThrowsAsync<RedisServerException> on the follow-up HashGetAllAsync. I traced the exception path to confirm this holds rather than assuming it: GarnetException(message, disposeSession: false) with the default clientResponse: true goes through RespServerSession's catch handler, which writes a real -ERR Garnet Exception: ... RESP error (not a connection drop) and leaves the session open since DisposeSession is false -- so RedisServerException is what the client actually sees, and the connection survives, which I added an assertion for.

One thing I want to be upfront about: I couldn't get a clean pass/fail out of this locally. ~4.7GB of hash data plus a client-side buffer needing to grow past it is more than my sandbox has room for. I tried a lighter local variant (6 x 400MB fields, same total ballpark, well under the 512MB per-value cap) to at least exercise the same code path faster, but that itself got OOM-killed by the sandbox's own memory limits partway through the writes, before ever reaching HGETALL -- so that's a limitation of my environment, not a finding about the code, and I pulled that throwaway variant back out rather than leave a noisy data point in. The reasoning above (traced through the actual exception-handling code, plus Array.MaxLength being ~2GB against a ~4.7GB payload) is why I'm fairly confident it'll pass, but CI is the first place this actually gets exercised end to end -- I'll keep an eye on the run.

After some internal discussion, we just want the simpler fix here. I've started putting that together in #1987 .