[Backport] Http.Sys: Clean up Request parsing errors

src/Servers/HttpSys/src/LoggerEventIds.cs

@@ -58,4 +58,5 @@ internal static class LoggerEventIds
     public const int AcceptSetExpectationMismatch = 51;
     public const int AcceptCancelExpectationMismatch = 52;
     public const int AcceptObserveExpectationMismatch = 53;
+    public const int RequestParsingError = 54;
 }

src/Servers/HttpSys/src/RequestProcessing/Request.cs

@@ -37,145 +37,152 @@ internal Request(RequestContext requestContext)
     {
         // TODO: Verbose log
         RequestContext = requestContext;
-        _contentBoundaryType = BoundaryType.None;
 
-        RequestId = requestContext.RequestId;
-        // For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection.
-        UConnectionId = requestContext.ConnectionId;
-        RawConnectionId = requestContext.RawConnectionId;
-        SslStatus = requestContext.SslStatus;
+        try
+        {
+            _contentBoundaryType = BoundaryType.None;
 
-        KnownMethod = requestContext.VerbId;
-        Method = requestContext.GetVerb()!;
+            RequestId = requestContext.RequestId;
+            // For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection.
+            UConnectionId = requestContext.ConnectionId;
+            RawConnectionId = requestContext.RawConnectionId;
+            SslStatus = requestContext.SslStatus;
 
-        RawUrl = requestContext.GetRawUrl()!;
+            KnownMethod = requestContext.VerbId;
+            Method = requestContext.GetVerb()!;
 
-        var cookedUrl = requestContext.GetCookedUrl();
-        QueryString = cookedUrl.GetQueryString() ?? string.Empty;
+            RawUrl = requestContext.GetRawUrl()!;
 
-        var rawUrlInBytes = requestContext.GetRawUrlInBytes();
-        var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes);
+            var cookedUrl = requestContext.GetCookedUrl();
+            QueryString = cookedUrl.GetQueryString() ?? string.Empty;
 
-        PathBase = string.Empty;
-        Path = originalPath;
-        var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext);
+            var rawUrlInBytes = requestContext.GetRawUrlInBytes();
+            var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes);
 
-        // 'OPTIONS * HTTP/1.1'
-        if (KnownMethod == HttpApiTypes.HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal))
-        {
             PathBase = string.Empty;
-            Path = string.Empty;
-        }
-        // Prefix may be null if the requested has been transfered to our queue
-        else if (prefix is not null)
-        {
-            var pathBase = prefix.PathWithoutTrailingSlash;
+            Path = originalPath;
+            var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext);
 
-            // url: /base/path, prefix: /base/, base: /base, path: /path
-            // url: /, prefix: /, base: , path: /
-            if (originalPath.Equals(pathBase, StringComparison.Ordinal))
-            {
-                // Exact match, no need to preserve the casing
-                PathBase = pathBase;
-                Path = string.Empty;
-            }
-            else if (originalPath.Equals(pathBase, StringComparison.OrdinalIgnoreCase))
+            // 'OPTIONS * HTTP/1.1'
+            if (KnownMethod == HttpApiTypes.HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal))
             {
-                // Preserve the user input casing
-                PathBase = originalPath;
+                PathBase = string.Empty;
                 Path = string.Empty;
             }
-            else if (originalPath.StartsWith(prefix.Path, StringComparison.Ordinal))
-            {
-                // Exact match, no need to preserve the casing
-                PathBase = pathBase;
-                Path = originalPath[pathBase.Length..];
-            }
-            else if (originalPath.StartsWith(prefix.Path, StringComparison.OrdinalIgnoreCase))
+            // Prefix may be null if the requested has been transferred to our queue
+            else if (prefix is not null)
             {
-                // Preserve the user input casing
-                PathBase = originalPath[..pathBase.Length];
-                Path = originalPath[pathBase.Length..];
-            }
-            else
-            {
-                // Http.Sys path base matching is based on the cooked url which applies some non-standard normalizations that we don't use
-                // like collapsing duplicate slashes "//", converting '\' to '/', and un-escaping "%2F" to '/'. Find the right split and
-                // ignore the normalizations.
-                var originalOffset = 0;
-                var baseOffset = 0;
-                while (originalOffset < originalPath.Length && baseOffset < pathBase.Length)
+                var pathBase = prefix.PathWithoutTrailingSlash;
+
+                // url: /base/path, prefix: /base/, base: /base, path: /path
+                // url: /, prefix: /, base: , path: /
+                if (originalPath.Equals(pathBase, StringComparison.Ordinal))
                 {
-                    var baseValue = pathBase[baseOffset];
-                    var offsetValue = originalPath[originalOffset];
-                    if (baseValue == offsetValue
-                        || char.ToUpperInvariant(baseValue) == char.ToUpperInvariant(offsetValue))
-                    {
-                        // case-insensitive match, continue
-                        originalOffset++;
-                        baseOffset++;
-                    }
-                    else if (baseValue == '/' && offsetValue == '\\')
-                    {
-                        // Http.Sys considers these equivalent
-                        originalOffset++;
-                        baseOffset++;
-                    }
-                    else if (baseValue == '/' && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
-                    {
-                        // Http.Sys un-escapes this
-                        originalOffset += 3;
-                        baseOffset++;
-                    }
-                    else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
-                        && (offsetValue == '/' || offsetValue == '\\'))
-                    {
-                        // Duplicate slash, skip
-                        originalOffset++;
-                    }
-                    else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
-                        && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
-                    {
-                        // Duplicate slash equivalent, skip
-                        originalOffset += 3;
-                    }
-                    else
+                    // Exact match, no need to preserve the casing
+                    PathBase = pathBase;
+                    Path = string.Empty;
+                }
+                else if (originalPath.Equals(pathBase, StringComparison.OrdinalIgnoreCase))
+                {
+                    // Preserve the user input casing
+                    PathBase = originalPath;
+                    Path = string.Empty;
+                }
+                else if (originalPath.StartsWith(prefix.Path, StringComparison.Ordinal))
+                {
+                    // Exact match, no need to preserve the casing
+                    PathBase = pathBase;
+                    Path = originalPath[pathBase.Length..];
+                }
+                else if (originalPath.StartsWith(prefix.Path, StringComparison.OrdinalIgnoreCase))
+                {
+                    // Preserve the user input casing
+                    PathBase = originalPath[..pathBase.Length];
+                    Path = originalPath[pathBase.Length..];
+                }
+                else
+                {
+                    // Http.Sys path base matching is based on the cooked url which applies some non-standard normalizations that we don't use
+                    // like collapsing duplicate slashes "//", converting '\' to '/', and un-escaping "%2F" to '/'. Find the right split and
+                    // ignore the normalizations.
+                    var originalOffset = 0;
+                    var baseOffset = 0;
+                    while (originalOffset < originalPath.Length && baseOffset < pathBase.Length)
                     {
-                        // Mismatch, fall back
-                        // The failing test case here is "/base/call//../bat//path1//path2", reduced to "/base/call/bat//path1//path2",
-                        // where http.sys collapses "//" before "../", but we do "../" first. We've lost the context that there were dot segments,
-                        // or duplicate slashes, how do we figure out that "call/" can be eliminated?
-                        originalOffset = 0;
-                        break;
+                        var baseValue = pathBase[baseOffset];
+                        var offsetValue = originalPath[originalOffset];
+                        if (baseValue == offsetValue
+                            || char.ToUpperInvariant(baseValue) == char.ToUpperInvariant(offsetValue))
+                        {
+                            // case-insensitive match, continue
+                            originalOffset++;
+                            baseOffset++;
+                        }
+                        else if (baseValue == '/' && offsetValue == '\\')
+                        {
+                            // Http.Sys considers these equivalent
+                            originalOffset++;
+                            baseOffset++;
+                        }
+                        else if (baseValue == '/' && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
+                        {
+                            // Http.Sys un-escapes this
+                            originalOffset += 3;
+                            baseOffset++;
+                        }
+                        else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
+                            && (offsetValue == '/' || offsetValue == '\\'))
+                        {
+                            // Duplicate slash, skip
+                            originalOffset++;
+                        }
+                        else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
+                            && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
+                        {
+                            // Duplicate slash equivalent, skip
+                            originalOffset += 3;
+                        }
+                        else
+                        {
+                            // Mismatch, fall back
+                            // The failing test case here is "/base/call//../bat//path1//path2", reduced to "/base/call/bat//path1//path2",
+                            // where http.sys collapses "//" before "../", but we do "../" first. We've lost the context that there were dot segments,
+                            // or duplicate slashes, how do we figure out that "call/" can be eliminated?
+                            originalOffset = 0;
+                            break;
+                        }
                     }
+                    PathBase = originalPath[..originalOffset];
+                    Path = originalPath[originalOffset..];
                 }
-                PathBase = originalPath[..originalOffset];
-                Path = originalPath[originalOffset..];
             }
-        }
-        else if (requestContext.Server.Options.UrlPrefixes.TryMatchLongestPrefix(IsHttps, cookedUrl.GetHost()!, originalPath, out var pathBase, out var path))
-        {
-            PathBase = pathBase;
-            Path = path;
-        }
+            else if (requestContext.Server.Options.UrlPrefixes.TryMatchLongestPrefix(IsHttps, cookedUrl.GetHost()!, originalPath, out var pathBase, out var path))
+            {
+                PathBase = pathBase;
+                Path = path;
+            }
 
-        ProtocolVersion = RequestContext.GetVersion();
+            ProtocolVersion = RequestContext.GetVersion();
 
-        Headers = new RequestHeaders(RequestContext);
+            Headers = new RequestHeaders(RequestContext);
 
-        User = RequestContext.GetUser();
+            User = RequestContext.GetUser();
 
-        SniHostName = string.Empty;
-        if (IsHttps)
-        {
-            GetTlsHandshakeResults();
-        }
+            SniHostName = string.Empty;
+            if (IsHttps)
+            {
+                GetTlsHandshakeResults();
+            }
 
-        // GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231
+            // GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231
 
-        // Finished directly accessing the HTTP_REQUEST structure.
-        RequestContext.ReleasePins();
-        // TODO: Verbose log parameters
+        }
+        finally
+        {
+            // Finished directly accessing the HTTP_REQUEST structure.
+            RequestContext.ReleasePins();
+            // TODO: Verbose log parameters
+        }
 
         RemoveContentLengthIfTransferEncodingContainsChunked();
     }

src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs

@@ -64,6 +64,7 @@ internal partial class RequestContext :
     private bool _bodyCompleted;
     private IHeaderDictionary _responseHeaders = default!;
     private IHeaderDictionary? _responseTrailers;
+    private ulong? _requestId;
 
     private Fields _initializedFields;
 
@@ -98,11 +99,26 @@ private enum Fields
         TraceIdentifier = 0x200,
     }
 
-    protected internal void InitializeFeatures()
+    protected internal bool InitializeFeatures()
     {
         _initialized = true;
 
-        Request = new Request(this);
+        // Get the ID before creating the Request object as the Request ctor releases the native memory
+        // We want the ID in case request processing fails and we need the ID to cancel the native request
+        _requestId = RequestId;
+        try
+        {
+            Request = new Request(this);
+        }
+        catch (Exception ex)
+        {
+            Log.RequestParsingError(Logger, ex);
+            // Synchronously calls Http.Sys and tells it to send an http response
+            // No one has written to the response yet (haven't even created the response object below)
+            Server.SendError(_requestId.Value, StatusCodes.Status400BadRequest, authChallenges: null);
+            return false;
+        }
+
         Response = new Response(this);
 
         _features = new FeatureCollection(new StandardFeatureCollection(this));
@@ -124,6 +140,7 @@ protected internal void InitializeFeatures()
 
         _responseStream = new ResponseStream(Response.Body, OnResponseStart);
         _responseHeaders = Response.Headers;
+        return true;
     }
 
     private bool IsNotInitialized(Fields field)

src/Servers/HttpSys/src/RequestProcessing/RequestContext.Log.cs

@@ -17,5 +17,8 @@ private static partial class Log
 
         [LoggerMessage(LoggerEventIds.ChannelBindingRetrieved, LogLevel.Debug, "Channel binding retrieved.", EventName = "ChannelBindingRetrieved")]
         public static partial void ChannelBindingRetrieved(ILogger logger);
+
+        [LoggerMessage(LoggerEventIds.RequestParsingError, LogLevel.Debug, "Failed to parse request.", EventName = "RequestParsingError")]
+        public static partial void RequestParsingError(ILogger logger, Exception exception);
     }
 }

src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs

@@ -187,9 +187,11 @@ public void Abort()
             _disconnectToken = new CancellationToken(canceled: true);
         }
         ForceCancelRequest();
-        Request.Dispose();
+        // Request and/or Response can be null (even though the property doesn't say it can)
+        // if the constructor throws (can happen for invalid path format)
+        Request?.Dispose();
         // Only Abort, Response.Dispose() tries a graceful flush
-        Response.Abort();
+        Response?.Abort();
     }
 
     private static void Abort(object? state)
@@ -208,15 +210,22 @@ internal void ForceCancelRequest()
     {
         try
         {
+            // Shouldn't be able to get here when this is null, but just in case we'll noop
+            if (_requestId is null)
+            {
+                return;
+            }
+
             var statusCode = HttpApi.HttpCancelHttpRequest(Server.RequestQueue.Handle,
-                Request.RequestId, IntPtr.Zero);
+                _requestId.Value, default);
 
             // Either the connection has already dropped, or the last write is in progress.
             // The requestId becomes invalid as soon as the last Content-Length write starts.
             // The only way to cancel now is with CancelIoEx.
             if (statusCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_CONNECTION_INVALID)
             {
-                Response.CancelLastWrite();
+                // Can be null if processing the request threw and the response object was never created.
+                Response?.CancelLastWrite();
             }
         }
         catch (ObjectDisposedException)

src/Servers/HttpSys/src/RequestProcessing/RequestContextOfT.cs

@@ -27,7 +27,11 @@ public override async Task ExecuteAsync()
 
         try
         {
-            InitializeFeatures();
+            if (!InitializeFeatures())
+            {
+                Abort();
+                return;
+            }
 
             if (messagePump.Stopping)
             {