Skip to content

Commit e718b11

Browse files
BrennanConroyBrennanConroy
committed
null checks
1 parent 5ce2ff6 commit e718b11

File tree

15 files changed

+161
-278
lines changed

15 files changed

+161
-278
lines changed

‎src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,8 @@ bool TrimAndTakeMessageHeaders(ref SequenceReader<byte> reader, bool trailers)
290290

291291
public void OnStartLine(HttpVersionAndMethod versionAndMethod, TargetOffsetPathLength targetPath, Span<byte> startLine)
292292
{
293+
Debug.Assert(startLine.IndexOf((byte)0) == -1);
294+
293295
var targetStart = targetPath.Offset;
294296
// Slice out target
295297
var target = startLine[targetStart..];
@@ -322,7 +324,7 @@ public void OnStartLine(HttpVersionAndMethod versionAndMethod, TargetOffsetPathL
322324
Method = method;
323325
if (method == HttpMethod.Custom)
324326
{
325-
_methodText = startLine[..versionAndMethod.MethodEnd].GetAsciiStringNonNullCharacters();
327+
_methodText = startLine[..versionAndMethod.MethodEnd].GetAsciiString();
326328
}
327329

328330
_httpVersion = versionAndMethod.Version;
@@ -386,7 +388,7 @@ private void ParseTarget(TargetOffsetPathLength targetPath, Span<byte> target)
386388
{
387389
// The previous string does not match what the bytes would convert to,
388390
// so we will need to generate a new string.
389-
RawTarget = _parsedRawTarget = target.GetAsciiStringNonNullCharacters();
391+
RawTarget = _parsedRawTarget = target.GetAsciiString();
390392

391393
var queryLength = 0;
392394
if (target.Length == targetPath.Length)
@@ -436,7 +438,7 @@ private int ParseQuery(TargetOffsetPathLength targetPath, Span<byte> target)
436438
{
437439
// The previous string does not match what the bytes would convert to,
438440
// so we will need to generate a new string.
439-
QueryString = _parsedQueryString = query.GetAsciiStringNonNullCharacters();
441+
QueryString = _parsedQueryString = query.GetAsciiString();
440442
}
441443
else
442444
{
@@ -482,7 +484,7 @@ private void OnAuthorityFormTarget(HttpMethod method, Span<byte> target)
482484
{
483485
// The previous string does not match what the bytes would convert to,
484486
// so we will need to generate a new string.
485-
RawTarget = _parsedRawTarget = target.GetAsciiStringNonNullCharacters();
487+
RawTarget = _parsedRawTarget = target.GetAsciiString();
486488
}
487489
else
488490
{
@@ -542,7 +544,7 @@ private void OnAbsoluteFormTarget(TargetOffsetPathLength targetPath, Span<byte>
542544
{
543545
// The previous string does not match what the bytes would convert to,
544546
// so we will need to generate a new string.
545-
RawTarget = _parsedRawTarget = target.GetAsciiStringNonNullCharacters();
547+
RawTarget = _parsedRawTarget = target.GetAsciiString();
546548
}
547549
catch (InvalidOperationException)
548550
{
@@ -572,7 +574,7 @@ private void OnAbsoluteFormTarget(TargetOffsetPathLength targetPath, Span<byte>
572574
{
573575
// The previous string does not match what the bytes would convert to,
574576
// so we will need to generate a new string.
575-
QueryString = _parsedQueryString = query.GetAsciiStringNonNullCharacters();
577+
QueryString = _parsedQueryString = query.GetAsciiString();
576578
}
577579
else
578580
{

‎src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,14 @@ public bool ParseRequestLine(TRequestHandler handler, ref SequenceReader<byte> r
6969
}
7070

7171
// Consume the delimiter.
72-
bool foundDelimiter = reader.TryRead(out var next);
72+
var foundDelimiter = reader.TryRead(out var next);
7373
Debug.Assert(foundDelimiter);
7474
if (next == 0 || requestLine.Length == 0)
7575
{
76+
reader.Rewind(requestLine.Length + 1);
77+
var readResult = reader.TryReadExact(requestLine.Length + 1, out var requestLineSequence);
78+
Debug.Assert(readResult);
79+
requestLine = requestLineSequence.IsSingleSegment ? requestLineSequence.FirstSpan : requestLineSequence.ToArray();
7680
RejectRequestLine(requestLine);
7781
}
7882

‎src/Servers/Kestrel/Core/src/Internal/Http/PathNormalizer.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public static string DecodePath(Span<byte> path, bool pathEncoded, string rawTar
4747
return rawTarget;
4848
}
4949

50-
return path.Slice(0, pathLength).GetAsciiStringNonNullCharacters();
50+
return path.Slice(0, pathLength).GetAsciiString();
5151
}
5252

5353
// In-place implementation of the algorithm from https://tools.ietf.org/html/rfc3986#section-5.2.4

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -86,37 +86,61 @@ private static ulong GetMaskAsLong(ReadOnlySpan<byte> bytes)
8686
[MethodImpl(MethodImplOptions.AggressiveInlining)]
8787
public static unsafe string GetHeaderName(this ReadOnlySpan<byte> span)
8888
{
89-
return string.Create(span.Length, (IntPtr)(&span), (destination, spanPtr) =>
89+
if (span.IsEmpty)
90+
{
91+
return string.Empty;
92+
}
93+
94+
var str = string.Create(span.Length, (IntPtr)(&span), (destination, spanPtr) =>
9095
{
9196
if (Ascii.ToUtf16(*(ReadOnlySpan<byte>*)spanPtr, destination, out _) != OperationStatus.Done)
9297
{
9398
KestrelBadHttpRequestException.Throw(RequestRejectionReason.InvalidCharactersInHeaderName);
9499
}
95100
});
101+
102+
if (str.Contains('\0'))
103+
{
104+
KestrelBadHttpRequestException.Throw(RequestRejectionReason.InvalidCharactersInHeaderName);
105+
}
106+
return str;
96107
}
97108

98-
public static string GetAsciiStringNonNullCharacters(this Span<byte> span)
99-
=> StringUtilities.GetAsciiStringNonNullCharacters(span);
109+
// Null checks must be done independently of this method (if required)
110+
public static string GetAsciiString(this Span<byte> span)
111+
=> StringUtilities.GetAsciiString(span);
100112

101-
public static string GetAsciiOrUTF8StringNonNullCharacters(this ReadOnlySpan<byte> span)
102-
=> StringUtilities.GetAsciiOrUTF8StringNonNullCharacters(span, DefaultRequestHeaderEncoding);
113+
public static string GetAsciiOrUTF8String(this ReadOnlySpan<byte> span)
114+
=> StringUtilities.GetAsciiOrUTF8String(span, DefaultRequestHeaderEncoding);
103115

104116
public static string GetRequestHeaderString(this ReadOnlySpan<byte> span, string name, Func<string, Encoding?> encodingSelector, bool checkForNewlineChars)
105117
{
106118
string result;
107119
if (ReferenceEquals(KestrelServerOptions.DefaultHeaderEncodingSelector, encodingSelector))
108120
{
109-
result = span.GetAsciiOrUTF8StringNonNullCharacters(DefaultRequestHeaderEncoding);
121+
result = span.GetAsciiOrUTF8String(DefaultRequestHeaderEncoding);
110122
}
111123
else
112124
{
113125
result = span.GetRequestHeaderStringWithoutDefaultEncodingCore(name, encodingSelector);
114126
}
115127

116128
// New Line characters (CR, LF) are considered invalid at this point.
117-
if (checkForNewlineChars && ((ReadOnlySpan<char>)result).IndexOfAny('\r', '\n') >= 0)
129+
// Null characters are also not allowed.
130+
var invalidCharIndex = checkForNewlineChars ?
131+
((ReadOnlySpan<char>)result).IndexOfAny('\r', '\n', '\0')
132+
: ((ReadOnlySpan<char>)result).IndexOf('\0');
133+
134+
if (invalidCharIndex >= 0)
118135
{
119-
throw new InvalidOperationException("Newline characters (CR/LF) are not allowed in request headers.");
136+
if (result[invalidCharIndex] == 0)
137+
{
138+
throw new InvalidOperationException("Null characters are not allowed in request headers.");
139+
}
140+
else
141+
{
142+
throw new InvalidOperationException("Newline characters (CR/LF) are not allowed in request headers.");
143+
}
120144
}
121145

122146
return result;
@@ -128,11 +152,11 @@ private static string GetRequestHeaderStringWithoutDefaultEncodingCore(this Read
128152

129153
if (encoding is null)
130154
{
131-
return span.GetAsciiOrUTF8StringNonNullCharacters(DefaultRequestHeaderEncoding);
155+
return span.GetAsciiOrUTF8String(DefaultRequestHeaderEncoding);
132156
}
133157
if (ReferenceEquals(encoding, Encoding.Latin1))
134158
{
135-
return span.GetLatin1StringNonNullCharacters();
159+
return span.GetLatin1String();
136160
}
137161
try
138162
{

‎src/Servers/Kestrel/Core/test/AsciiDecoding.cs

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ private void FullAsciiRangeSupported()
3535

3636
static void Test(Span<byte> asciiBytes)
3737
{
38-
var s = asciiBytes.GetAsciiStringNonNullCharacters();
38+
var s = asciiBytes.GetAsciiString();
3939

4040
Assert.True(StringUtilities.BytesOrdinalEqualsStringAndAscii(s, asciiBytes));
4141
Assert.Equal(s.Length, asciiBytes.Length);
@@ -50,19 +50,18 @@ static void Test(Span<byte> asciiBytes)
5050
}
5151
}
5252

53-
[Theory]
54-
[InlineData(0x00)]
55-
[InlineData(0x80)]
56-
private void ExceptionThrownForZeroOrNonAscii(byte b)
53+
[Fact]
54+
private void ExceptionThrownForNonAscii()
5755
{
56+
byte b = 0x80;
5857
for (var length = 1; length < Vector<sbyte>.Count * 4; length++)
5958
{
6059
for (var position = 0; position < length; position++)
6160
{
6261
var byteRange = Enumerable.Range(1, length).Select(x => (byte)x).ToArray();
6362
byteRange[position] = b;
6463

65-
Assert.Throws<InvalidOperationException>(() => new Span<byte>(byteRange).GetAsciiStringNonNullCharacters());
64+
Assert.Throws<InvalidOperationException>(() => new Span<byte>(byteRange).GetAsciiString());
6665
}
6766
}
6867
}
@@ -74,7 +73,7 @@ private void LargeAllocationProducesCorrectResults()
7473
var expectedByteRange = byteRange.Concat(byteRange).ToArray();
7574

7675
var span = new Span<byte>(expectedByteRange);
77-
var s = span.GetAsciiStringNonNullCharacters();
76+
var s = span.GetAsciiString();
7877

7978
Assert.Equal(expectedByteRange.Length, s.Length);
8079

@@ -96,7 +95,7 @@ private void DifferentLengthsAreNotEqual()
9695
for (var i = 1; i < byteRange.Length; i++)
9796
{
9897
var span = new Span<byte>(expectedByteRange);
99-
var s = span.GetAsciiStringNonNullCharacters();
98+
var s = span.GetAsciiString();
10099

101100
Assert.True(StringUtilities.BytesOrdinalEqualsStringAndAscii(s, span));
102101

@@ -133,7 +132,7 @@ private void AsciiBytesEqualAsciiStrings()
133132

134133
static void Test(Span<byte> asciiBytes)
135134
{
136-
var s = asciiBytes.GetAsciiStringNonNullCharacters();
135+
var s = asciiBytes.GetAsciiString();
137136

138137
// Should start as equal
139138
Assert.True(StringUtilities.BytesOrdinalEqualsStringAndAscii(s, asciiBytes));

‎src/Servers/Kestrel/Core/test/Http1/Http1ConnectionTests.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,8 @@ public async Task TakeStartLineThrowsOnNullCharInTarget(string target)
494494
TakeStartLine(readableBuffer, out _consumed, out _examined));
495495
_transport.Input.AdvanceTo(_consumed, _examined);
496496

497-
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestTarget_Detail(target.EscapeNonPrintable()), exception.Message);
497+
var partialTarget = target.AsSpan(0, target.IndexOf('\0') + 1).ToString();
498+
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail($"GET {partialTarget.EscapeNonPrintable()}"), exception.Message);
498499
}
499500

500501
[Theory]
@@ -510,7 +511,8 @@ public async Task TakeStartLineThrowsOnNullCharInMethod(string method)
510511
TakeStartLine(readableBuffer, out _consumed, out _examined));
511512
_transport.Input.AdvanceTo(_consumed, _examined);
512513

513-
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(requestLine[..^1].EscapeNonPrintable()), exception.Message);
514+
var partialRequestLine = requestLine.AsSpan(0, requestLine.IndexOf('\0') + 1).ToString();
515+
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(partialRequestLine.EscapeNonPrintable()), exception.Message);
514516
}
515517

516518
[Theory]
@@ -526,7 +528,8 @@ public async Task TakeStartLineThrowsOnNullCharInQueryString(string queryString)
526528
TakeStartLine(readableBuffer, out _consumed, out _examined));
527529
_transport.Input.AdvanceTo(_consumed, _examined);
528530

529-
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestTarget_Detail(target.EscapeNonPrintable()), exception.Message);
531+
var partialTarget = target.AsSpan(0, target.IndexOf('\0') + 1).ToString();
532+
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail($"GET {partialTarget.EscapeNonPrintable()}"), exception.Message);
530533
}
531534

532535
[Theory]

‎src/Servers/Kestrel/Core/test/HttpParserTests.cs

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,13 @@ public void ParseRequestLineThrowsOnInvalidRequestLine(string requestLine)
8787
#pragma warning restore CS0618 // Type or member is obsolete
8888
ParseRequestLine(parser, requestHandler, buffer, out var consumed, out var examined));
8989

90-
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(requestLine[..^1].EscapeNonPrintable()), exception.Message);
90+
var line = requestLine;
91+
var nullIndex = line.IndexOf('\0');
92+
if (nullIndex >= 0)
93+
{
94+
line = line.AsSpan().Slice(0, nullIndex + 2).ToString();
95+
}
96+
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(line[..^1].EscapeNonPrintable()), exception.Message);
9197
Assert.Equal(StatusCodes.Status400BadRequest, exception.StatusCode);
9298
}
9399

@@ -134,7 +140,16 @@ public void ParseRequestLineThrowsOnNonTokenCharsInCustomMethod(string method)
134140
#pragma warning restore CS0618 // Type or member is obsolete
135141
ParseRequestLine(parser, requestHandler, buffer, out var consumed, out var examined));
136142

137-
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(method.EscapeNonPrintable() + @" / HTTP/1.1\x0D"), exception.Message);
143+
var nullIndex = method.IndexOf('\0');
144+
if (nullIndex >= 0)
145+
{
146+
var line = method.AsSpan().Slice(0, nullIndex + 1).ToString();
147+
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(line.EscapeNonPrintable()), exception.Message);
148+
}
149+
else
150+
{
151+
Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestLine_Detail(method.EscapeNonPrintable() + @" / HTTP/1.1\x0D"), exception.Message);
152+
}
138153
Assert.Equal(StatusCodes.Status400BadRequest, exception.StatusCode);
139154
}
140155

@@ -871,18 +886,18 @@ private class RequestHandler : IHttpRequestLineHandler, IHttpHeadersHandler
871886

872887
public void OnHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
873888
{
874-
Headers[name.GetAsciiStringNonNullCharacters()] = value.GetAsciiStringNonNullCharacters();
889+
Headers[name.GetAsciiString()] = value.GetAsciiString();
875890
}
876891

877892
void IHttpHeadersHandler.OnHeadersComplete(bool endStream) { }
878893

879894
public void OnStartLine(HttpMethod method, HttpVersion version, Span<byte> target, Span<byte> path, Span<byte> query, Span<byte> customMethod, bool pathEncoded)
880895
{
881-
Method = method != HttpMethod.Custom ? HttpUtilities.MethodToString(method) : customMethod.GetAsciiStringNonNullCharacters();
896+
Method = method != HttpMethod.Custom ? HttpUtilities.MethodToString(method) : customMethod.GetAsciiString();
882897
Version = HttpUtilities.VersionToString(version);
883-
RawTarget = target.GetAsciiStringNonNullCharacters();
884-
RawPath = path.GetAsciiStringNonNullCharacters();
885-
Query = query.GetAsciiStringNonNullCharacters();
898+
RawTarget = target.GetAsciiString();
899+
RawPath = path.GetAsciiString();
900+
Query = query.GetAsciiString();
886901
PathEncoded = pathEncoded;
887902
}
888903

@@ -896,11 +911,11 @@ public void OnStartLine(HttpVersionAndMethod versionAndMethod, TargetOffsetPathL
896911
var path = target[..targetPath.Length];
897912
var query = target[targetPath.Length..];
898913

899-
Method = method != HttpMethod.Custom ? HttpUtilities.MethodToString(method) : customMethod.GetAsciiStringNonNullCharacters();
914+
Method = method != HttpMethod.Custom ? HttpUtilities.MethodToString(method) : customMethod.GetAsciiString();
900915
Version = HttpUtilities.VersionToString(version);
901-
RawTarget = target.GetAsciiStringNonNullCharacters();
902-
RawPath = path.GetAsciiStringNonNullCharacters();
903-
Query = query.GetAsciiStringNonNullCharacters();
916+
RawTarget = target.GetAsciiString();
917+
RawPath = path.GetAsciiString();
918+
Query = query.GetAsciiString();
904919
PathEncoded = targetPath.IsEncoded;
905920
}
906921

‎src/Servers/Kestrel/perf/Microbenchmarks/BytesToStringBenchmark.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public void AsciiBytesToString()
6565
{
6666
for (uint i = 0; i < Iterations; i++)
6767
{
68-
HttpUtilities.GetAsciiStringNonNullCharacters(_asciiBytes);
68+
HttpUtilities.GetAsciiString(_asciiBytes);
6969
}
7070
}
7171

‎src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -771,7 +771,7 @@ internal async Task ExpectReceiveEndOfStream()
771771

772772
public void OnHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
773773
{
774-
_headerHandler.DecodedHeaders[name.GetAsciiStringNonNullCharacters()] = value.GetAsciiOrUTF8StringNonNullCharacters();
774+
_headerHandler.DecodedHeaders[name.GetAsciiString()] = value.GetAsciiOrUTF8String();
775775
}
776776

777777
public void OnHeadersComplete(bool endHeaders)
@@ -781,12 +781,12 @@ public void OnHeadersComplete(bool endHeaders)
781781
public void OnStaticIndexedHeader(int index)
782782
{
783783
var knownHeader = H3StaticTable.Get(index);
784-
_headerHandler.DecodedHeaders[((Span<byte>)knownHeader.Name).GetAsciiStringNonNullCharacters()] = HttpUtilities.GetAsciiOrUTF8StringNonNullCharacters((ReadOnlySpan<byte>)knownHeader.Value);
784+
_headerHandler.DecodedHeaders[((Span<byte>)knownHeader.Name).GetAsciiString()] = HttpUtilities.GetAsciiOrUTF8String((ReadOnlySpan<byte>)knownHeader.Value);
785785
}
786786

787787
public void OnStaticIndexedHeader(int index, ReadOnlySpan<byte> value)
788788
{
789-
_headerHandler.DecodedHeaders[((Span<byte>)H3StaticTable.Get(index).Name).GetAsciiStringNonNullCharacters()] = value.GetAsciiOrUTF8StringNonNullCharacters();
789+
_headerHandler.DecodedHeaders[((Span<byte>)H3StaticTable.Get(index).Name).GetAsciiString()] = value.GetAsciiOrUTF8String();
790790
}
791791

792792
public void Complete()
@@ -796,7 +796,7 @@ public void Complete()
796796

797797
public void OnDynamicIndexedHeader(int? index, ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
798798
{
799-
_headerHandler.DecodedHeaders[name.GetAsciiStringNonNullCharacters()] = value.GetAsciiOrUTF8StringNonNullCharacters();
799+
_headerHandler.DecodedHeaders[name.GetAsciiString()] = value.GetAsciiOrUTF8String();
800800
}
801801
}
802802

0 commit comments

Comments
 (0)