Skip to content

Commit 37dd88b

Browse files
BrennanConroyBrennanConroy
authored
Http.Sys: Clean up Request parsing errors (dotnet#57531)
* Http.Sys: Clean up Request parsing errors * nit * comment
1 parent 296bdde commit 37dd88b

File tree

10 files changed

+219
-138
lines changed

10 files changed

+219
-138
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
@@ -38,145 +38,152 @@ internal Request(RequestContext requestContext)
3838
{
3939
// TODO: Verbose log
4040
RequestContext = requestContext;
41-
_contentBoundaryType = BoundaryType.None;
4241

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

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

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

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

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

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

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

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

163-
ProtocolVersion = RequestContext.GetVersion();
166+
ProtocolVersion = RequestContext.GetVersion();
164167

165-
Headers = new RequestHeaders(RequestContext);
168+
Headers = new RequestHeaders(RequestContext);
166169

167-
User = RequestContext.GetUser();
170+
User = RequestContext.GetUser();
168171

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

175-
// GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231
178+
// GetTlsTokenBindingInfo(); TODO: https://github.com/aspnet/HttpSysServer/issues/231
176179

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

181188
RemoveContentLengthIfTransferEncodingContainsChunked();
182189
}

‎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
@@ -171,9 +171,11 @@ public void Abort()
171171
_disconnectToken = new CancellationToken(canceled: true);
172172
}
173173
ForceCancelRequest();
174-
Request.Dispose();
174+
// Request and/or Response can be null (even though the property doesn't say it can)
175+
// if the constructor throws (can happen for invalid path format)
176+
Request?.Dispose();
175177
// Only Abort, Response.Dispose() tries a graceful flush
176-
Response.Abort();
178+
Response?.Abort();
177179
}
178180

179181
private static void Abort(object? state)
@@ -192,15 +194,22 @@ internal void ForceCancelRequest()
192194
{
193195
try
194196
{
197+
// Shouldn't be able to get here when this is null, but just in case we'll noop
198+
if (_requestId is null)
199+
{
200+
return;
201+
}
202+
195203
var statusCode = PInvoke.HttpCancelHttpRequest(Server.RequestQueue.Handle,
196-
Request.RequestId, default);
204+
_requestId.Value, default);
197205

198206
// Either the connection has already dropped, or the last write is in progress.
199207
// The requestId becomes invalid as soon as the last Content-Length write starts.
200208
// The only way to cancel now is with CancelIoEx.
201209
if (statusCode == ErrorCodes.ERROR_CONNECTION_INVALID)
202210
{
203-
Response.CancelLastWrite();
211+
// Can be null if processing the request threw and the response object was never created.
212+
Response?.CancelLastWrite();
204213
}
205214
}
206215
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)