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