Skip to content

Commit dfe2137

Browse files
BrennanConroyBrennanConroy
authored
[Backport] Http.Sys: Clean up Request parsing errors (#57819)
* Http.Sys: Clean up Request parsing errors * nit * comment * fix
1 parent cec987b commit dfe2137

File tree

10 files changed

+219
-137
lines changed

10 files changed

+219
-137
lines changed

‎src/Servers/HttpSys/src/LoggerEventIds.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,4 +58,5 @@ internal static class LoggerEventIds
5858
public const int AcceptSetExpectationMismatch = 51;
5959
public const int AcceptCancelExpectationMismatch = 52;
6060
public const int AcceptObserveExpectationMismatch = 53;
61+
public const int RequestParsingError = 54;
6162
}

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

Lines changed: 122 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -37,145 +37,152 @@ internal Request(RequestContext requestContext)
3737
{
3838
// TODO: Verbose log
3939
RequestContext = requestContext;
40-
_contentBoundaryType = BoundaryType.None;
4140

42-
RequestId = requestContext.RequestId;
43-
// For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection.
44-
UConnectionId = requestContext.ConnectionId;
45-
RawConnectionId = requestContext.RawConnectionId;
46-
SslStatus = requestContext.SslStatus;
41+
try
42+
{
43+
_contentBoundaryType = BoundaryType.None;
4744

48-
KnownMethod = requestContext.VerbId;
49-
Method = requestContext.GetVerb()!;
45+
RequestId = requestContext.RequestId;
46+
// For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection.
47+
UConnectionId = requestContext.ConnectionId;
48+
RawConnectionId = requestContext.RawConnectionId;
49+
SslStatus = requestContext.SslStatus;
5050

51-
RawUrl = requestContext.GetRawUrl()!;
51+
KnownMethod = requestContext.VerbId;
52+
Method = requestContext.GetVerb()!;
5253

53-
var cookedUrl = requestContext.GetCookedUrl();
54-
QueryString = cookedUrl.GetQueryString() ?? string.Empty;
54+
RawUrl = requestContext.GetRawUrl()!;
5555

56-
var rawUrlInBytes = requestContext.GetRawUrlInBytes();
57-
var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes);
56+
var cookedUrl = requestContext.GetCookedUrl();
57+
QueryString = cookedUrl.GetQueryString() ?? string.Empty;
5858

59-
PathBase = string.Empty;
60-
Path = originalPath;
61-
var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext);
59+
var rawUrlInBytes = requestContext.GetRawUrlInBytes();
60+
var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes);
6261

63-
// 'OPTIONS * HTTP/1.1'
64-
if (KnownMethod == HttpApiTypes.HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal))
65-
{
6662
PathBase = string.Empty;
67-
Path = string.Empty;
68-
}
69-
// Prefix may be null if the requested has been transfered to our queue
70-
else if (prefix is not null)
71-
{
72-
var pathBase = prefix.PathWithoutTrailingSlash;
63+
Path = originalPath;
64+
var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext);
7365

74-
// url: /base/path, prefix: /base/, base: /base, path: /path
75-
// url: /, prefix: /, base: , path: /
76-
if (originalPath.Equals(pathBase, StringComparison.Ordinal))
77-
{
78-
// Exact match, no need to preserve the casing
79-
PathBase = pathBase;
80-
Path = string.Empty;
81-
}
82-
else if (originalPath.Equals(pathBase, StringComparison.OrdinalIgnoreCase))
66+
// 'OPTIONS * HTTP/1.1'
67+
if (KnownMethod == HttpApiTypes.HTTP_VERB.HttpVerbOPTIONS && string.Equals(RawUrl, "*", StringComparison.Ordinal))
8368
{
84-
// Preserve the user input casing
85-
PathBase = originalPath;
69+
PathBase = string.Empty;
8670
Path = string.Empty;
8771
}
88-
else if (originalPath.StartsWith(prefix.Path, StringComparison.Ordinal))
89-
{
90-
// Exact match, no need to preserve the casing
91-
PathBase = pathBase;
92-
Path = originalPath[pathBase.Length..];
93-
}
94-
else if (originalPath.StartsWith(prefix.Path, StringComparison.OrdinalIgnoreCase))
72+
// Prefix may be null if the requested has been transferred to our queue
73+
else if (prefix is not null)
9574
{
96-
// Preserve the user input casing
97-
PathBase = originalPath[..pathBase.Length];
98-
Path = originalPath[pathBase.Length..];
99-
}
100-
else
101-
{
102-
// Http.Sys path base matching is based on the cooked url which applies some non-standard normalizations that we don't use
103-
// like collapsing duplicate slashes "//", converting '\' to '/', and un-escaping "%2F" to '/'. Find the right split and
104-
// ignore the normalizations.
105-
var originalOffset = 0;
106-
var baseOffset = 0;
107-
while (originalOffset < originalPath.Length && baseOffset < pathBase.Length)
75+
var pathBase = prefix.PathWithoutTrailingSlash;
76+
77+
// url: /base/path, prefix: /base/, base: /base, path: /path
78+
// url: /, prefix: /, base: , path: /
79+
if (originalPath.Equals(pathBase, StringComparison.Ordinal))
10880
{
109-
var baseValue = pathBase[baseOffset];
110-
var offsetValue = originalPath[originalOffset];
111-
if (baseValue == offsetValue
112-
|| char.ToUpperInvariant(baseValue) == char.ToUpperInvariant(offsetValue))
113-
{
114-
// case-insensitive match, continue
115-
originalOffset++;
116-
baseOffset++;
117-
}
118-
else if (baseValue == '/' && offsetValue == '\\')
119-
{
120-
// Http.Sys considers these equivalent
121-
originalOffset++;
122-
baseOffset++;
123-
}
124-
else if (baseValue == '/' && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
125-
{
126-
// Http.Sys un-escapes this
127-
originalOffset += 3;
128-
baseOffset++;
129-
}
130-
else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
131-
&& (offsetValue == '/' || offsetValue == '\\'))
132-
{
133-
// Duplicate slash, skip
134-
originalOffset++;
135-
}
136-
else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
137-
&& originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
138-
{
139-
// Duplicate slash equivalent, skip
140-
originalOffset += 3;
141-
}
142-
else
81+
// Exact match, no need to preserve the casing
82+
PathBase = pathBase;
83+
Path = string.Empty;
84+
}
85+
else if (originalPath.Equals(pathBase, StringComparison.OrdinalIgnoreCase))
86+
{
87+
// Preserve the user input casing
88+
PathBase = originalPath;
89+
Path = string.Empty;
90+
}
91+
else if (originalPath.StartsWith(prefix.Path, StringComparison.Ordinal))
92+
{
93+
// Exact match, no need to preserve the casing
94+
PathBase = pathBase;
95+
Path = originalPath[pathBase.Length..];
96+
}
97+
else if (originalPath.StartsWith(prefix.Path, StringComparison.OrdinalIgnoreCase))
98+
{
99+
// Preserve the user input casing
100+
PathBase = originalPath[..pathBase.Length];
101+
Path = originalPath[pathBase.Length..];
102+
}
103+
else
104+
{
105+
// Http.Sys path base matching is based on the cooked url which applies some non-standard normalizations that we don't use
106+
// like collapsing duplicate slashes "//", converting '\' to '/', and un-escaping "%2F" to '/'. Find the right split and
107+
// ignore the normalizations.
108+
var originalOffset = 0;
109+
var baseOffset = 0;
110+
while (originalOffset < originalPath.Length && baseOffset < pathBase.Length)
143111
{
144-
// Mismatch, fall back
145-
// The failing test case here is "/base/call//../bat//path1//path2", reduced to "/base/call/bat//path1//path2",
146-
// where http.sys collapses "//" before "../", but we do "../" first. We've lost the context that there were dot segments,
147-
// or duplicate slashes, how do we figure out that "call/" can be eliminated?
148-
originalOffset = 0;
149-
break;
112+
var baseValue = pathBase[baseOffset];
113+
var offsetValue = originalPath[originalOffset];
114+
if (baseValue == offsetValue
115+
|| char.ToUpperInvariant(baseValue) == char.ToUpperInvariant(offsetValue))
116+
{
117+
// case-insensitive match, continue
118+
originalOffset++;
119+
baseOffset++;
120+
}
121+
else if (baseValue == '/' && offsetValue == '\\')
122+
{
123+
// Http.Sys considers these equivalent
124+
originalOffset++;
125+
baseOffset++;
126+
}
127+
else if (baseValue == '/' && originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
128+
{
129+
// Http.Sys un-escapes this
130+
originalOffset += 3;
131+
baseOffset++;
132+
}
133+
else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
134+
&& (offsetValue == '/' || offsetValue == '\\'))
135+
{
136+
// Duplicate slash, skip
137+
originalOffset++;
138+
}
139+
else if (baseOffset > 0 && pathBase[baseOffset - 1] == '/'
140+
&& originalPath.AsSpan(originalOffset).StartsWith("%2F", StringComparison.OrdinalIgnoreCase))
141+
{
142+
// Duplicate slash equivalent, skip
143+
originalOffset += 3;
144+
}
145+
else
146+
{
147+
// Mismatch, fall back
148+
// The failing test case here is "/base/call//../bat//path1//path2", reduced to "/base/call/bat//path1//path2",
149+
// where http.sys collapses "//" before "../", but we do "../" first. We've lost the context that there were dot segments,
150+
// or duplicate slashes, how do we figure out that "call/" can be eliminated?
151+
originalOffset = 0;
152+
break;
153+
}
150154
}
155+
PathBase = originalPath[..originalOffset];
156+
Path = originalPath[originalOffset..];
151157
}
152-
PathBase = originalPath[..originalOffset];
153-
Path = originalPath[originalOffset..];
154158
}
155-
}
156-
else if (requestContext.Server.Options.UrlPrefixes.TryMatchLongestPrefix(IsHttps, cookedUrl.GetHost()!, originalPath, out var pathBase, out var path))
157-
{
158-
PathBase = pathBase;
159-
Path = path;
160-
}
159+
else if (requestContext.Server.Options.UrlPrefixes.TryMatchLongestPrefix(IsHttps, cookedUrl.GetHost()!, originalPath, out var pathBase, out var path))
160+
{
161+
PathBase = pathBase;
162+
Path = path;
163+
}
161164

162-
ProtocolVersion = RequestContext.GetVersion();
165+
ProtocolVersion = RequestContext.GetVersion();
163166

164-
Headers = new RequestHeaders(RequestContext);
167+
Headers = new RequestHeaders(RequestContext);
165168

166-
User = RequestContext.GetUser();
169+
User = RequestContext.GetUser();
167170

168-
SniHostName = string.Empty;
169-
if (IsHttps)
170-
{
171-
GetTlsHandshakeResults();
172-
}
171+
SniHostName = string.Empty;
172+
if (IsHttps)
173+
{
174+
GetTlsHandshakeResults();
175+
}
173176

174-
// GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231
177+
// GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231
175178

176-
// Finished directly accessing the HTTP_REQUEST structure.
177-
RequestContext.ReleasePins();
178-
// TODO: Verbose log parameters
179+
}
180+
finally
181+
{
182+
// Finished directly accessing the HTTP_REQUEST structure.
183+
RequestContext.ReleasePins();
184+
// TODO: Verbose log parameters
185+
}
179186

180187
RemoveContentLengthIfTransferEncodingContainsChunked();
181188
}

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ internal partial class RequestContext :
6464
private bool _bodyCompleted;
6565
private IHeaderDictionary _responseHeaders = default!;
6666
private IHeaderDictionary? _responseTrailers;
67+
private ulong? _requestId;
6768

6869
private Fields _initializedFields;
6970

@@ -98,11 +99,26 @@ private enum Fields
9899
TraceIdentifier = 0x200,
99100
}
100101

101-
protected internal void InitializeFeatures()
102+
protected internal bool InitializeFeatures()
102103
{
103104
_initialized = true;
104105

105-
Request = new Request(this);
106+
// Get the ID before creating the Request object as the Request ctor releases the native memory
107+
// We want the ID in case request processing fails and we need the ID to cancel the native request
108+
_requestId = RequestId;
109+
try
110+
{
111+
Request = new Request(this);
112+
}
113+
catch (Exception ex)
114+
{
115+
Log.RequestParsingError(Logger, ex);
116+
// Synchronously calls Http.Sys and tells it to send an http response
117+
// No one has written to the response yet (haven't even created the response object below)
118+
Server.SendError(_requestId.Value, StatusCodes.Status400BadRequest, authChallenges: null);
119+
return false;
120+
}
121+
106122
Response = new Response(this);
107123

108124
_features = new FeatureCollection(new StandardFeatureCollection(this));
@@ -124,6 +140,7 @@ protected internal void InitializeFeatures()
124140

125141
_responseStream = new ResponseStream(Response.Body, OnResponseStart);
126142
_responseHeaders = Response.Headers;
143+
return true;
127144
}
128145

129146
private bool IsNotInitialized(Fields field)

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,8 @@ private static partial class Log
1717

1818
[LoggerMessage(LoggerEventIds.ChannelBindingRetrieved, LogLevel.Debug, "Channel binding retrieved.", EventName = "ChannelBindingRetrieved")]
1919
public static partial void ChannelBindingRetrieved(ILogger logger);
20+
21+
[LoggerMessage(LoggerEventIds.RequestParsingError, LogLevel.Debug, "Failed to parse request.", EventName = "RequestParsingError")]
22+
public static partial void RequestParsingError(ILogger logger, Exception exception);
2023
}
2124
}

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

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,11 @@ public void Abort()
187187
_disconnectToken = new CancellationToken(canceled: true);
188188
}
189189
ForceCancelRequest();
190-
Request.Dispose();
190+
// Request and/or Response can be null (even though the property doesn't say it can)
191+
// if the constructor throws (can happen for invalid path format)
192+
Request?.Dispose();
191193
// Only Abort, Response.Dispose() tries a graceful flush
192-
Response.Abort();
194+
Response?.Abort();
193195
}
194196

195197
private static void Abort(object? state)
@@ -208,15 +210,22 @@ internal void ForceCancelRequest()
208210
{
209211
try
210212
{
213+
// Shouldn't be able to get here when this is null, but just in case we'll noop
214+
if (_requestId is null)
215+
{
216+
return;
217+
}
218+
211219
var statusCode = HttpApi.HttpCancelHttpRequest(Server.RequestQueue.Handle,
212-
Request.RequestId, IntPtr.Zero);
220+
_requestId.Value, default);
213221

214222
// Either the connection has already dropped, or the last write is in progress.
215223
// The requestId becomes invalid as soon as the last Content-Length write starts.
216224
// The only way to cancel now is with CancelIoEx.
217225
if (statusCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_CONNECTION_INVALID)
218226
{
219-
Response.CancelLastWrite();
227+
// Can be null if processing the request threw and the response object was never created.
228+
Response?.CancelLastWrite();
220229
}
221230
}
222231
catch (ObjectDisposedException)

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,11 @@ public override async Task ExecuteAsync()
2727

2828
try
2929
{
30-
InitializeFeatures();
30+
if (!InitializeFeatures())
31+
{
32+
Abort();
33+
return;
34+
}
3135

3236
if (messagePump.Stopping)
3337
{

0 commit comments

Comments
 (0)