diff --git a/src/IceRpc.Deadline/DeadlineInterceptor.cs b/src/IceRpc.Deadline/DeadlineInterceptor.cs index 1782029fc7..2990e9ac47 100644 --- a/src/IceRpc.Deadline/DeadlineInterceptor.cs +++ b/src/IceRpc.Deadline/DeadlineInterceptor.cs @@ -25,6 +25,9 @@ namespace IceRpc.Deadline; /// public class DeadlineInterceptor : IInvoker { + // The maximum delay CancellationTokenSource.CancelAfter(TimeSpan) accepts. + private static readonly TimeSpan _maxCancelAfterDelay = TimeSpan.FromMilliseconds(int.MaxValue); + private readonly bool _alwaysEnforceDeadline; private readonly IInvoker _next; private readonly TimeSpan _defaultTimeout; @@ -32,15 +35,22 @@ public class DeadlineInterceptor : IInvoker /// Constructs a Deadline interceptor. /// The next invoker in the invocation pipeline. - /// The default timeout. When not infinite, the interceptor adds a deadline to requests - /// without a deadline. - /// The optional time provider used to obtain the current time. If , it uses - /// . + /// The default timeout applied to requests without a deadline. Must be positive and + /// must not exceed milliseconds (~24.8 days), the maximum supported by + /// , or to + /// disable this behavior. + /// The optional time provider used to obtain the current time. If + /// , it uses . /// When and the request carries a deadline, the /// interceptor always creates a cancellation token source to enforce this deadline. When /// and the request carries a deadline, the interceptor creates a cancellation token source to enforce this deadline /// only when the invocation's cancellation token cannot be canceled. The default value is . /// + /// Thrown if is not + /// , not positive, or exceeds the maximum supported value. + /// A request carrying an whose computed remaining timeout exceeds + /// the maximum (~24.8 days) is silently clamped to + /// that maximum at invocation time. public DeadlineInterceptor(IInvoker next, TimeSpan defaultTimeout, bool alwaysEnforceDeadline, TimeProvider? timeProvider = null) { if (defaultTimeout != Timeout.InfiniteTimeSpan && defaultTimeout <= TimeSpan.Zero) @@ -50,6 +60,13 @@ public DeadlineInterceptor(IInvoker next, TimeSpan defaultTimeout, bool alwaysEn nameof(defaultTimeout)); } + if (defaultTimeout != Timeout.InfiniteTimeSpan && defaultTimeout > _maxCancelAfterDelay) + { + throw new ArgumentException( + $"The {nameof(defaultTimeout)} value must not exceed {_maxCancelAfterDelay} or be Timeout.InfiniteTimeSpan.", + nameof(defaultTimeout)); + } + _next = next; _alwaysEnforceDeadline = alwaysEnforceDeadline; _defaultTimeout = defaultTimeout; @@ -96,6 +113,13 @@ public Task InvokeAsync(OutgoingRequest request, CancellationT async Task PerformInvokeAsync(TimeSpan timeout) { + // Clamp to CancelAfter's supported maximum. A caller-provided IDeadlineFeature value near + // DateTime.MaxValue would otherwise cause CancelAfter to throw ArgumentOutOfRangeException. + if (timeout > _maxCancelAfterDelay) + { + timeout = _maxCancelAfterDelay; + } + using var timeoutTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); timeoutTokenSource.CancelAfter(timeout); diff --git a/src/IceRpc.Deadline/DeadlineMiddleware.cs b/src/IceRpc.Deadline/DeadlineMiddleware.cs index 067e6d3801..4627c29c31 100644 --- a/src/IceRpc.Deadline/DeadlineMiddleware.cs +++ b/src/IceRpc.Deadline/DeadlineMiddleware.cs @@ -2,6 +2,7 @@ using IceRpc.Extensions.DependencyInjection; using IceRpc.Features; +using System.Buffers; using ZeroC.Slice.Codec; namespace IceRpc.Deadline; @@ -9,10 +10,16 @@ namespace IceRpc.Deadline; /// Represents a middleware that decodes deadline fields into deadline features. When the decoded deadline /// expires, this middleware cancels the dispatch and returns an with status code /// . +/// A peer-encoded deadline whose computed remaining timeout exceeds the +/// maximum (~24.8 days) is silently clamped to that +/// maximum. /// /// public class DeadlineMiddleware : IDispatcher { + // The maximum delay CancellationTokenSource.CancelAfter(TimeSpan) accepts. + private static readonly TimeSpan _maxCancelAfterDelay = TimeSpan.FromMilliseconds(int.MaxValue); + private readonly IDispatcher _next; private readonly TimeProvider _timeProvider; @@ -31,16 +38,12 @@ public ValueTask DispatchAsync( IncomingRequest request, CancellationToken cancellationToken = default) { - TimeSpan? timeout = null; - - // not found returns default == DateTime.MinValue. - DateTime deadline = request.Fields.DecodeValue( - RequestFieldKey.Deadline, - (ref SliceDecoder decoder) => decoder.DecodeTimeStamp()); - - if (deadline != DateTime.MinValue) + // Check explicit field presence rather than relying on a decoded-value sentinel. + if (request.Fields.TryGetValue(RequestFieldKey.Deadline, out ReadOnlySequence value)) { - timeout = deadline - _timeProvider.GetUtcNow().UtcDateTime; + DateTime deadline = value.DecodeSliceBuffer( + (ref SliceDecoder decoder) => decoder.DecodeTimeStamp()); + TimeSpan timeout = deadline - _timeProvider.GetUtcNow().UtcDateTime; if (timeout <= TimeSpan.Zero) { @@ -50,10 +53,19 @@ public ValueTask DispatchAsync( "The request deadline has expired.")); } + // Clamp to CancelAfter's supported maximum. A peer-encoded deadline thousands of years in the future + // would otherwise cause CancelAfter to throw ArgumentOutOfRangeException, surfacing as a generic + // InternalError response. + if (timeout > _maxCancelAfterDelay) + { + timeout = _maxCancelAfterDelay; + } + request.Features = request.Features.With(new DeadlineFeature(deadline)); + return PerformDispatchAsync(timeout); } - return timeout is null ? _next.DispatchAsync(request, cancellationToken) : PerformDispatchAsync(timeout.Value); + return _next.DispatchAsync(request, cancellationToken); async ValueTask PerformDispatchAsync(TimeSpan timeout) { diff --git a/src/IceRpc/Features/DeadlineFeature.cs b/src/IceRpc/Features/DeadlineFeature.cs index 54b44654c2..e40e645489 100644 --- a/src/IceRpc/Features/DeadlineFeature.cs +++ b/src/IceRpc/Features/DeadlineFeature.cs @@ -5,10 +5,32 @@ namespace IceRpc.Features; /// Default implementation of . public sealed class DeadlineFeature : IDeadlineFeature { + // The maximum delay CancellationTokenSource.CancelAfter(TimeSpan) accepts. + private static readonly TimeSpan MaxCancelAfterDelay = TimeSpan.FromMilliseconds(int.MaxValue); + /// Creates a deadline from a timeout. - /// The timeout. + /// The timeout. Must be positive and must not exceed + /// milliseconds (~24.8 days), the maximum supported by + /// . /// A new deadline equal to now plus the timeout. - public static DeadlineFeature FromTimeout(TimeSpan timeout) => new(DateTime.UtcNow + timeout); + /// Thrown if is not positive, or exceeds the + /// maximum supported value. + public static DeadlineFeature FromTimeout(TimeSpan timeout) + { + if (timeout <= TimeSpan.Zero) + { + throw new ArgumentException( + $"The {nameof(timeout)} value must be positive.", + nameof(timeout)); + } + if (timeout > MaxCancelAfterDelay) + { + throw new ArgumentException( + $"The {nameof(timeout)} value must not exceed {MaxCancelAfterDelay}.", + nameof(timeout)); + } + return new(DateTime.UtcNow + timeout); + } /// public DateTime Value { get; } diff --git a/tests/IceRpc.Deadline.Tests/DeadlineInterceptorTests.cs b/tests/IceRpc.Deadline.Tests/DeadlineInterceptorTests.cs index 1d354c14dc..cc2941d34a 100644 --- a/tests/IceRpc.Deadline.Tests/DeadlineInterceptorTests.cs +++ b/tests/IceRpc.Deadline.Tests/DeadlineInterceptorTests.cs @@ -135,6 +135,28 @@ public async Task Deadline_interceptor_does_not_enforce_deadline_by_default() Assert.That(token!.Value, Is.EqualTo(cts.Token)); } + /// Verifies the interceptor clamps an extreme-future IDeadlineFeature value instead of letting + /// CancelAfter throw ArgumentOutOfRangeException. + [Test] + public async Task Invoke_with_extreme_future_deadline_does_not_throw() + { + // Arrange + var invoker = new InlineInvoker((request, cancellationToken) => + Task.FromResult(new IncomingResponse(request, FakeConnectionContext.Instance))); + + var sut = new DeadlineInterceptor(invoker, Timeout.InfiniteTimeSpan, alwaysEnforceDeadline: true); + // Close to DateTime.MaxValue but not equal, so the interceptor's "== DateTime.MaxValue" short-circuit + // does not kick in and the CancelAfter path is actually exercised. + DateTime extreme = DateTime.SpecifyKind(DateTime.MaxValue.AddDays(-1), DateTimeKind.Utc); + using var request = new OutgoingRequest(new ServiceAddress(Protocol.IceRpc)) + { + Features = new FeatureCollection().With(new DeadlineFeature(extreme)) + }; + + // Act/Assert + Assert.That(async () => await sut.InvokeAsync(request, default), Throws.Nothing); + } + [Test] public void Deadline_interceptor_can_enforce_application_deadline() { @@ -229,6 +251,20 @@ public void Constructor_rejects_invalid_default_timeout(int milliseconds) Throws.TypeOf()); } + /// Verifies the constructor rejects a default timeout beyond CancelAfter's supported maximum. + /// TimeSpan.MaxValue would otherwise later cause DateTime overflow (now + timeout) or CancelAfter to throw + /// ArgumentOutOfRangeException at first invoke. + [Test] + public void Constructor_rejects_default_timeout_beyond_cancel_after_max() + { + var invoker = new InlineInvoker((request, cancellationToken) => + Task.FromResult(new IncomingResponse(request, FakeConnectionContext.Instance))); + + Assert.That( + () => new DeadlineInterceptor(invoker, TimeSpan.MaxValue, alwaysEnforceDeadline: false), + Throws.TypeOf()); + } + [Test] public void Constructor_accepts_infinite_timeout() { diff --git a/tests/IceRpc.Deadline.Tests/DeadlineMiddlewareTests.cs b/tests/IceRpc.Deadline.Tests/DeadlineMiddlewareTests.cs index 26f1e7ddce..c7ad4527b2 100644 --- a/tests/IceRpc.Deadline.Tests/DeadlineMiddlewareTests.cs +++ b/tests/IceRpc.Deadline.Tests/DeadlineMiddlewareTests.cs @@ -56,6 +56,82 @@ public async Task Dispatch_fails_after_the_deadline_expires() pipeReader.Complete(); } + /// Verifies that a request with an explicitly encoded DateTime.MinValue deadline returns + /// DeadlineExceeded and does not fall through to the next dispatcher. This is the decoded form of a peer-encoded + /// ticks=0 deadline, which must be treated as an expired deadline rather than as an absent field. + [Test] + public async Task Dispatch_fails_when_deadline_is_min_value() + { + // Arrange + bool nextCalled = false; + var dispatcher = new InlineDispatcher((request, cancellationToken) => + { + nextCalled = true; + return new(new OutgoingResponse(request)); + }); + + var sut = new DeadlineMiddleware(dispatcher); + + // Encode an explicit ticks=0 deadline: the Utc kind avoids ToUniversalTime shifting it away from 0 in + // timezones other than UTC. Decoded server-side as DateTime.MinValue, which collides with the "field + // absent" sentinel the middleware used to rely on. + PipeReader pipeReader = WriteDeadline(new DateTime(0, DateTimeKind.Utc)); + pipeReader.TryRead(out var readResult); + + using var request = new IncomingRequest(Protocol.IceRpc, FakeConnectionContext.Instance) + { + Fields = new Dictionary> + { + [RequestFieldKey.Deadline] = readResult.Buffer + } + }; + + // Act + OutgoingResponse response = await sut.DispatchAsync(request, CancellationToken.None); + + // Assert + Assert.That(response.StatusCode, Is.EqualTo(StatusCode.DeadlineExceeded)); + Assert.That(nextCalled, Is.False); + + // Cleanup + pipeReader.Complete(); + } + + /// Verifies the middleware clamps an extreme-future peer-encoded deadline instead of letting + /// CancelAfter throw ArgumentOutOfRangeException. + [Test] + public async Task Dispatch_with_extreme_future_deadline_does_not_throw() + { + // Arrange + bool nextCalled = false; + var dispatcher = new InlineDispatcher((request, cancellationToken) => + { + nextCalled = true; + return new(new OutgoingResponse(request)); + }); + + var sut = new DeadlineMiddleware(dispatcher); + + PipeReader pipeReader = WriteDeadline( + DateTime.SpecifyKind(DateTime.MaxValue, DateTimeKind.Utc)); + pipeReader.TryRead(out var readResult); + + using var request = new IncomingRequest(Protocol.IceRpc, FakeConnectionContext.Instance) + { + Fields = new Dictionary> + { + [RequestFieldKey.Deadline] = readResult.Buffer + } + }; + + // Act/Assert + Assert.That(async () => await sut.DispatchAsync(request, CancellationToken.None), Throws.Nothing); + Assert.That(nextCalled, Is.True); + + // Cleanup + pipeReader.Complete(); + } + /// Verifies that the deadline decoded by the middleware has the expected value. [Test] public async Task Deadline_decoded_by_middleware_has_expected_value() diff --git a/tests/IceRpc.Tests/Features/DeadlineFeatureTests.cs b/tests/IceRpc.Tests/Features/DeadlineFeatureTests.cs new file mode 100644 index 0000000000..64239f5cac --- /dev/null +++ b/tests/IceRpc.Tests/Features/DeadlineFeatureTests.cs @@ -0,0 +1,24 @@ +// Copyright (c) ZeroC, Inc. + +using NUnit.Framework; + +namespace IceRpc.Features.Tests; + +[Parallelizable(scope: ParallelScope.All)] +public class DeadlineFeatureTests +{ + [Test] + public void FromTimeout_rejects_non_positive_timeout() + { + Assert.That(() => DeadlineFeature.FromTimeout(TimeSpan.Zero), Throws.TypeOf()); + Assert.That(() => DeadlineFeature.FromTimeout(TimeSpan.FromSeconds(-1)), Throws.TypeOf()); + } + + /// Verifies FromTimeout rejects a timeout beyond CancelAfter's supported maximum instead of + /// letting DateTime.UtcNow + timeout overflow with ArgumentOutOfRangeException. + [Test] + public void FromTimeout_rejects_timeout_beyond_cancel_after_max() + { + Assert.That(() => DeadlineFeature.FromTimeout(TimeSpan.MaxValue), Throws.TypeOf()); + } +}