Fix GETEX tearing down the session on an out-of-range expiry - #2094
Open
hexonal (hexonal) wants to merge 1 commit into
Open
Fix GETEX tearing down the session on an out-of-range expiry#2094hexonal (hexonal) wants to merge 1 commit into
hexonal (hexonal) wants to merge 1 commit into
Conversation
GETEX is the only command that turns its expiry option into a relative TimeSpan via TimeSpan.FromSeconds/FromMilliseconds (EX/PX) or an absolute instant via DateTimeOffset.FromUnixTimeSeconds/Milliseconds (EXAT/PXAT). Each of those throws on a value it cannot represent - an OverflowException for TimeSpan, an ArgumentOutOfRangeException past year 9999 for DateTimeOffset - and the only guard was expireTime <= 0. Neither exception is a RespParsingException or a GarnetException, so it escaped to the RespServerSession catch-all and disposed the network sender, dropping the client connection. A single "GETEX k EXAT 99999999999999" from any client tears down the session. There was a second, quieter bug in the same spot: for EX/PX values large enough to convert without throwing but past DateTimeOffset.MaxValue, the absolute expiry computed as UtcNow.Ticks + tsExpiry.Ticks silently overflowed the long into a negative instant, so GETEX applied a garbage TTL and replied with the value instead of an error. Bound every option by DateTimeOffset.MaxValue (year 9999) and reply with "ERR invalid expire time in 'getex' command", matching Redis, instead of crashing or overflowing. Values within the representable range - including far-future ones that worked before - are unaffected. Adds a LightClientRequest regression test whose trailing PING proves the connection survives; it covers all four options for the throwing case and EX/PX for the silent-overflow case, and fails on main (four with a Disconnected exception, two with the wrong reply).
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents out-of-range GETEX expiries from terminating client sessions or overflowing expiry calculations.
Changes:
- Validates all
GETEXexpiry options against representable limits. - Adds a command-specific RESP error.
- Tests rejection, connection survival, and unchanged TTL.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
libs/server/Resp/BasicCommands.cs |
Adds expiry bounds validation. |
libs/server/Resp/CmdStrings.cs |
Adds the GETEX expiry error. |
test/standalone/Garnet.test/RespTests.cs |
Covers overflow cases and session survival. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // computed below as UtcNow.Ticks + tsExpiry.Ticks does not silently overflow the | ||
| // long. Both are bounded by refusing any expiry beyond DateTimeOffset.MaxValue | ||
| // (year 9999), reporting an error the way Redis does rather than crashing. | ||
| var maxDeltaTicks = DateTimeOffset.MaxValue.Ticks - DateTimeOffset.UtcNow.Ticks; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
GETEXis the only command that converts its expiry option into a relativeTimeSpan(TimeSpan.FromSeconds/FromMillisecondsforEX/PX) or an absolute instant (DateTimeOffset.FromUnixTimeSeconds/FromUnixTimeMillisecondsforEXAT/PXAT). Each of those throws on a value it cannot represent — anOverflowExceptionforTimeSpan, anArgumentOutOfRangeExceptionpast year 9999 forDateTimeOffset— and the only guard inNetworkGETEXwasexpireTime <= 0.Neither exception is a
RespParsingExceptionor aGarnetException, so it falls through to theRespServerSessioncatch-all, which callsnetworkSender.Dispose()— dropping the client connection. Any client can tear down its session with a one-liner:There is a second, quieter bug in the same spot: for
EX/PXvalues large enough to convert without throwing but pastDateTimeOffset.MaxValue, the absolute expiry computed asUtcNow.Ticks + tsExpiry.Tickssilently overflows thelonginto a negative instant, soGETEXapplies a garbage TTL and replies with the value instead of an error (e.g.GETEX k EX 900000000000).Fix
Bound every option by
DateTimeOffset.MaxValue(year 9999) and reply with-ERR invalid expire time in 'getex' command, matching Redis, instead of crashing or overflowing.EX/PXare bounded relative toUtcNowso the resulting absolute expiry stays representable;EXAT/PXATare bounded by the absolute Unix ceiling. Values within range — including far-future ones that already worked — are unaffected.Test
Adds
GetExpiryOutOfRangeIsRejectedWithoutKillingSession, aLightClientRequesttest whose trailingPINGproves the connection survives (a disposed session never answers it). It covers all four options for the throwing case andEX/PXfor the silent-overflow case.On unpatched
mainall six cases fail — four withGarnetException: Disconnected, two with the wrong reply ($6\r\nvalueA\r\ninstead of the error). With the fix the fullGetExpiry/SetAndGetExpirysuite is green (32/32). Verified in Release on .NET 10.