Fix LREM with count = int.MinValue killing the session - #2009
Conversation
ListRemove normalizes a negative count with Math.Abs, which throws
OverflowException for int.MinValue ("Negating the minimum value of a
twos complement number is invalid"). The exception unwinds out of
ProcessMessages and terminates the RESP session: the client gets no
reply and a reset connection. The server process stays up and other
sessions are unaffected.
Reproduced against unmodified main:
RPUSH L a b c
LREM L -2147483648 a -> connection closed, no reply
Clamp instead of negating. The removal loop is bounded by
removedCount < count with removedCount never exceeding the list length,
which cannot exceed int.MaxValue, so int.MaxValue removes exactly the
same elements that a correctly negated 2147483648 would.
Scope: the other Math.Abs call sites on a client-supplied count
(HRANDFIELD, SRANDMEMBER, ZRANDMEMBER) were checked with the same input
and none of them terminates the session, so this is the only instance
and the fix stays at one line.
Verified on macOS arm64, net10.0 Release: the new test fails without the
change with RedisConnectionException SocketClosed, and passes with it.
RespListTests: 104/104. net8.0 builds clean; its tests were not run
here, no net8.0 runtime in this environment.
There was a problem hiding this comment.
Pull request overview
Fixes a crash/connection-drop edge case in the List object implementation when handling LREM with count = int.MinValue, which previously could throw OverflowException and terminate the issuing RESP session.
Changes:
- Clamp
counttoint.MaxValuewhencount == int.MinValueto avoidMath.Abs(int.MinValue)overflow while preserving removal semantics and direction. - Add a regression test ensuring
LREMwithint.MinValueremoves all matches and keeps the connection usable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libs/server/Objects/List/ListObjectImpl.cs | Prevents OverflowException by clamping int.MinValue before normalization in ListRemove. |
| test/standalone/Garnet.test.collections/RespListTests.cs | Adds a regression test validating the fix and confirming the session remains active after the call. |
|
The red check on 19 tests failed: 18 in with The branch sits on the current main tip (26b1882) with no other commits, and main's own runs are green. Running the full I do not have permission to re-run the job. Could someone re-trigger it? |
LREM key -2147483648 <element>terminates the RESP session.Server log:
The server process stays up and other sessions are unaffected — the blast radius is the issuing connection.
Cause
ListRemovenormalizes a negative count withMath.Abs(count), which throws forint.MinValuebecause|int.MinValue|does not fit in anint.Fix
Clamp rather than negate. The removal loop is
while (removedCount < count && currentNode != null), andremovedCountcan never exceed the list length, which cannot exceedint.MaxValue— soint.MaxValueremoves exactly the same elements that a correctly negated2147483648would. Direction is unaffected:fromHeadToTailis decided from the original sign before the clamp.Scope
I checked the other
Math.Abscall sites that take a client-supplied count —HRANDFIELD(HashObjectImpl.cs:138),SRANDMEMBER(SetObjectImpl.cs:219) andZRANDMEMBER(SortedSetObjectImpl.cs:651,656) — by driving each with-2147483648against a live server. None of them terminates the session, soLREMis the only instance and the production change stays at one line.Testing
LREMWithIntMinValueCountRemovesAllMatches: fails without the change withRedisConnectionException : SocketClosed (ReadEndOfStream), passes with it.RespListTests: 104/104.LREM key -2147483648 aona b a c areturns3and leavesb c;LREM key -2 astill returns2and leavesa b c(tail-first order preserved);LREM key 2147483647 aunchanged.dotnet format --verify-no-changesclean on both files.net8.0builds clean but its tests were not run here — no net8.0 runtime in this environment.