From 61d09c7242007a9e0d2fa4446807bb53841f424e Mon Sep 17 00:00:00 2001 From: hexonal Date: Mon, 31 Aug 2026 01:25:31 -0400 Subject: [PATCH] Fix GETEX tearing down the session on an out-of-range expiry 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). --- libs/server/Resp/BasicCommands.cs | 17 ++++++++++++ libs/server/Resp/CmdStrings.cs | 1 + test/standalone/Garnet.test/RespTests.cs | 35 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/libs/server/Resp/BasicCommands.cs b/libs/server/Resp/BasicCommands.cs index 8b25229045b..f10f5131c7d 100644 --- a/libs/server/Resp/BasicCommands.cs +++ b/libs/server/Resp/BasicCommands.cs @@ -115,21 +115,38 @@ bool NetworkGETEX(ref TGarnetApi storageApi) if (parseState.Count < 3 || !parseState.TryGetLong(2, out var expireTime) || expireTime <= 0) return AbortWithErrorMessage(CmdStrings.RESP_ERR_GENERIC_VALUE_IS_OUT_OF_RANGE); + // The expiry a GETEX option implies must land within the representable range so + // that (a) the conversion itself does not throw - TimeSpan.From* raises an + // OverflowException and DateTimeOffset.FromUnixTime* an ArgumentOutOfRangeException, + // neither a RespParsingException nor a GarnetException, so it would escape to the + // session catch-all and dispose the connection - and (b) the absolute expiry + // 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; switch (option) { case var _ when option.EqualsUpperCaseSpanIgnoringCase(CmdStrings.EX): + if (expireTime > maxDeltaTicks / TimeSpan.TicksPerSecond) + return AbortWithErrorMessage(CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_GETEX); tsExpiry = TimeSpan.FromSeconds(expireTime); break; case var _ when option.EqualsUpperCaseSpanIgnoringCase(CmdStrings.PX): + if (expireTime > maxDeltaTicks / TimeSpan.TicksPerMillisecond) + return AbortWithErrorMessage(CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_GETEX); tsExpiry = TimeSpan.FromMilliseconds(expireTime); break; case var _ when option.EqualsUpperCaseSpanIgnoringCase(CmdStrings.EXAT): + if (expireTime > DateTimeOffset.MaxValue.ToUnixTimeSeconds()) + return AbortWithErrorMessage(CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_GETEX); tsExpiry = DateTimeOffset.FromUnixTimeSeconds(expireTime) - DateTimeOffset.UtcNow; break; case var _ when option.EqualsUpperCaseSpanIgnoringCase(CmdStrings.PXAT): + if (expireTime > DateTimeOffset.MaxValue.ToUnixTimeMilliseconds()) + return AbortWithErrorMessage(CmdStrings.RESP_ERR_GENERIC_INVALIDEXP_IN_GETEX); tsExpiry = DateTimeOffset.FromUnixTimeMilliseconds(expireTime) - DateTimeOffset.UtcNow; break; diff --git a/libs/server/Resp/CmdStrings.cs b/libs/server/Resp/CmdStrings.cs index c04278d1eb7..1ab34958ec3 100644 --- a/libs/server/Resp/CmdStrings.cs +++ b/libs/server/Resp/CmdStrings.cs @@ -221,6 +221,7 @@ static partial class CmdStrings public static ReadOnlySpan RESP_ERR_GENERIC_DISCARD_WO_MULTI => "ERR DISCARD without MULTI"u8; public static ReadOnlySpan RESP_ERR_GENERIC_WATCH_IN_MULTI => "ERR WATCH inside MULTI is not allowed"u8; public static ReadOnlySpan RESP_ERR_GENERIC_INVALIDEXP_IN_SET => "ERR invalid expire time in 'set' command"u8; + public static ReadOnlySpan RESP_ERR_GENERIC_INVALIDEXP_IN_GETEX => "ERR invalid expire time in 'getex' command"u8; public static ReadOnlySpan RESP_ERR_GENERIC_SYNTAX_ERROR => "ERR syntax error"u8; public static ReadOnlySpan RESP_ERR_GENERIC_NAN_INFINITY => "ERR value is NaN or Infinity"u8; public static ReadOnlySpan RESP_ERR_GENERIC_NAN_INFINITY_INCR => "ERR increment would produce NaN or Infinity"u8; diff --git a/test/standalone/Garnet.test/RespTests.cs b/test/standalone/Garnet.test/RespTests.cs index eb54dccdd25..0fa48eb9c60 100644 --- a/test/standalone/Garnet.test/RespTests.cs +++ b/test/standalone/Garnet.test/RespTests.cs @@ -4964,6 +4964,41 @@ public void GetExpiryWitInvalidOptions(string optionsInput) Assert.Throws(() => db.Execute("GETEX", [key, .. options])); } + /// + /// An out-of-range expiry must be answered by an error and nothing else, and must leave the + /// connection usable. Two failure modes are covered: + /// - A value large enough that the conversion throws (EX/PX overflow , + /// EXAT/PXAT run past its year-9999 + /// ceiling). That exception used to escape to the session catch-all and tear the + /// connection down - the trailing PING is the tell, a disposed session never answers it. + /// - A value in the gap below that ceiling but past DateTimeOffset.MaxValue (the last two + /// cases): the conversion did not throw, but UtcNow.Ticks + delta silently overflowed the + /// long into a negative absolute expiry, so GETEX applied a garbage TTL and replied with + /// the value instead of an error. + /// + [Test] + [TestCase("EX 99999999999999")] + [TestCase("PX 99999999999999999")] + [TestCase("EXAT 99999999999999")] + [TestCase("PXAT 99999999999999999")] + [TestCase("EX 900000000000")] + [TestCase("PX 900000000000000")] + public void GetExpiryOutOfRangeIsRejectedWithoutKillingSession(string optionAndValue) + { + using var lightClientRequest = TestUtils.CreateRequest(); + + var expectedResponse = "-ERR invalid expire time in 'getex' command\r\n+PONG\r\n"; + + lightClientRequest.SendCommand("SET keyA valueA"); + + var response = lightClientRequest.SendCommands($"GETEX keyA {optionAndValue}", "PING", 1, 1); + TestUtils.AssertEqualUpToExpectedLength(expectedResponse, response); + + // The rejected expiry must not have been applied. + response = lightClientRequest.SendCommand("TTL keyA"); + TestUtils.AssertEqualUpToExpectedLength(":-1\r\n", response); + } + #endregion #region GETSET