Skip to content

Commit 723b0ab

Browse files
BrennanConroyBrennanConroy
authored
[Backport] Http.Sys: Clean up Request parsing errors (#57812)
* Http.Sys: Clean up Request parsing errors * nit * comment * merge fix * fix
1 parent 580e8ea commit 723b0ab

File tree

9 files changed

+202
-134
lines changed

9 files changed

+202
-134
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,5 +55,6 @@ internal static class LoggerEventIds
5555
public static readonly EventId ListenerDisposing = new EventId(46, "ListenerDisposing");
5656
public static readonly EventId RequestValidationFailed = new EventId(47, "RequestValidationFailed");
5757
public static readonly EventId CreateDisconnectTokenError = new EventId(48, "CreateDisconnectTokenError");
58+
public static readonly EventId RequestParsingError = new EventId(54, "RequestParsingError");
5859
}
5960
}

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

Lines changed: 121 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -42,143 +42,150 @@ internal Request(RequestContext requestContext)
4242
{
4343
// TODO: Verbose log
4444
RequestContext = requestContext;
45-
_contentBoundaryType = BoundaryType.None;
4645

47-
RequestId = requestContext.RequestId;
48-
// For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection.
49-
UConnectionId = requestContext.ConnectionId;
50-
RawConnectionId = requestContext.RawConnectionId;
51-
SslStatus = requestContext.SslStatus;
46+
try
47+
{
48+
_contentBoundaryType = BoundaryType.None;
5249

53-
KnownMethod = requestContext.VerbId;
54-
Method = requestContext.GetVerb()!;
50+
RequestId = requestContext.RequestId;
51+
// For HTTP/2 Http.Sys assigns each request a unique connection id for use with API calls, but the RawConnectionId represents the real connection.
52+
UConnectionId = requestContext.ConnectionId;
53+
RawConnectionId = requestContext.RawConnectionId;
54+
SslStatus = requestContext.SslStatus;
5555

56-
RawUrl = requestContext.GetRawUrl()!;
56+
KnownMethod = requestContext.VerbId;
57+
Method = requestContext.GetVerb()!;
5758

58-
var cookedUrl = requestContext.GetCookedUrl();
59-
QueryString = cookedUrl.GetQueryString() ?? string.Empty;
59+
RawUrl = requestContext.GetRawUrl()!;
6060

61-
var rawUrlInBytes = requestContext.GetRawUrlInBytes();
62-
var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes);
61+
var cookedUrl = requestContext.GetCookedUrl();
62+
QueryString = cookedUrl.GetQueryString() ?? string.Empty;
6363

64-
PathBase = string.Empty;
65-
Path = originalPath;
66-
var prefix = requestContext.Server.Options.UrlPrefixes.GetPrefix((int)requestContext.UrlContext);
64+
var rawUrlInBytes = requestContext.GetRawUrlInBytes();
65+
var originalPath = RequestUriBuilder.DecodeAndUnescapePath(rawUrlInBytes);
6766

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

166-
ProtocolVersion = RequestContext.GetVersion();
169+
ProtocolVersion = RequestContext.GetVersion();
167170

168-
Headers = new RequestHeaders(RequestContext);
171+
Headers = new RequestHeaders(RequestContext);
169172

170-
User = RequestContext.GetUser();
173+
User = RequestContext.GetUser();
171174

172-
if (IsHttps)
173-
{
174-
GetTlsHandshakeResults();
175-
}
175+
if (IsHttps)
176+
{
177+
GetTlsHandshakeResults();
178+
}
176179

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

179-
// Finished directly accessing the HTTP_REQUEST structure.
180-
RequestContext.ReleasePins();
181-
// TODO: Verbose log parameters
182+
}
183+
finally
184+
{
185+
// Finished directly accessing the HTTP_REQUEST structure.
186+
RequestContext.ReleasePins();
187+
// TODO: Verbose log parameters
188+
}
182189
}
183190

184191
internal ulong UConnectionId { get; }

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ internal partial class RequestContext :
6969
private bool _bodyCompleted;
7070
private IHeaderDictionary _responseHeaders = default!;
7171
private IHeaderDictionary? _responseTrailers;
72+
private ulong? _requestId;
7273

7374
private Fields _initializedFields;
7475

@@ -103,11 +104,26 @@ private enum Fields
103104
TraceIdentifier = 0x200,
104105
}
105106

106-
protected internal void InitializeFeatures()
107+
protected internal bool InitializeFeatures()
107108
{
108109
_initialized = true;
109110

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

113129
_features = new FeatureCollection(new StandardFeatureCollection(this));
@@ -129,6 +145,7 @@ protected internal void InitializeFeatures()
129145

130146
_responseStream = new ResponseStream(Response.Body, OnResponseStart);
131147
_responseHeaders = Response.Headers;
148+
return true;
132149
}
133150

134151

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ private static class Log
1919
private static readonly Action<ILogger, Exception?> _channelBindingRetrieved =
2020
LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.ChannelBindingRetrieved, "Channel binding retrieved.");
2121

22+
private static readonly Action<ILogger, Exception?> _requestParsingError =
23+
LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.RequestParsingError, "Failed to parse request.");
24+
2225
public static void AbortError(ILogger logger, Exception exception)
2326
{
2427
_abortError(logger, exception);
@@ -33,6 +36,11 @@ public static void ChannelBindingRetrieved(ILogger logger)
3336
{
3437
_channelBindingRetrieved(logger, null);
3538
}
39+
40+
public static void RequestParsingError(ILogger logger, Exception exception)
41+
{
42+
_requestParsingError(logger, exception);
43+
}
3644
}
3745
}
3846
}

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

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

194196
private static void Abort(object? state)
@@ -207,15 +209,22 @@ internal void ForceCancelRequest()
207209
{
208210
try
209211
{
212+
// Shouldn't be able to get here when this is null, but just in case we'll noop
213+
if (_requestId is null)
214+
{
215+
return;
216+
}
217+
210218
var statusCode = HttpApi.HttpCancelHttpRequest(Server.RequestQueue.Handle,
211-
Request.RequestId, IntPtr.Zero);
219+
_requestId.Value, IntPtr.Zero);
212220

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

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

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

3030
try
3131
{
32-
InitializeFeatures();
32+
if (!InitializeFeatures())
33+
{
34+
Abort();
35+
return;
36+
}
3337

3438
if (messagePump.Stopping)
3539
{

0 commit comments

Comments
 (0)