Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix Loop in System.Private.Uri #38076

Merged
wtgodbe merged 3 commits into from
May 31, 2019
Merged

Fix Loop in System.Private.Uri #38076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97bacab
Fix Loop in System.Private.Uri
wtgodbe May 16, 2019
8383171
Mark variables in test project as private
wtgodbe May 20, 2019
badad2d
Disable test on NetFx
wtgodbe May 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions src/System.Private.Uri/src/System/Uri.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2538,7 +2538,7 @@ private unsafe void CreateHostString()
}
else
{
for (ushort i = 0; i < host.Length; ++i)
for (int i = 0; i < host.Length; ++i)
{
if ((_info.Offset.Host + i) >= _info.Offset.End ||
host[i] != _string[_info.Offset.Host + i])
Expand Down Expand Up @@ -2655,7 +2655,7 @@ private unsafe void GetHostViaCustomSyntax()
else
{
host = CreateHostStringHelper(host, 0, (ushort)host.Length, ref flags, ref _info.ScopeId);
for (ushort i = 0; i < host.Length; ++i)
for (int i = 0; i < host.Length; ++i)
{
if ((_info.Offset.Host + i) >= _info.Offset.End || host[i] != _string[_info.Offset.Host + i])
{
Expand Down Expand Up @@ -3404,6 +3404,12 @@ private unsafe void ParseRemaining()
throw e;
}

if (_string.Length > ushort.MaxValue)
{
UriFormatException e = GetException(ParsingError.SizeLimit);
throw e;
}

length = (ushort)_string.Length;
// We need to be sure that there isn't a '?' separated from the path by spaces.
if (_string == _originalUnicodeString)
Expand Down Expand Up @@ -3536,6 +3542,12 @@ private unsafe void ParseRemaining()
throw e;
}

if (_string.Length > ushort.MaxValue)
{
UriFormatException e = GetException(ParsingError.SizeLimit);
throw e;
}

length = (ushort)_string.Length;
// We need to be sure that there isn't a '#' separated from the query by spaces.
if (_string == _originalUnicodeString)
Expand Down Expand Up @@ -3598,6 +3610,12 @@ private unsafe void ParseRemaining()
throw e;
}

if (_string.Length > ushort.MaxValue)
{
UriFormatException e = GetException(ParsingError.SizeLimit);
throw e;
}

length = (ushort)_string.Length;
// we don't need to check _originalUnicodeString == _string because # is last part
GetLengthWithoutTrailingSpaces(_string, ref length, idx);
Expand Down Expand Up @@ -4093,6 +4111,12 @@ private unsafe ushort CheckAuthorityHelper(char* pString, ushort idx, ushort len
// Normalize user info
userInfoString = IriHelper.EscapeUnescapeIri(pString, startInput, start + 1, UriComponents.UserInfo);
newHost += userInfoString;

if (newHost.Length > ushort.MaxValue)
{
err = ParsingError.SizeLimit;
return idx;
}
}
else
{
Expand Down
27 changes: 24 additions & 3 deletions src/System.Private.Uri/src/System/UriExt.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,16 @@ private void InitializeUri(ParsingError err, UriKind uriKind, out UriFormatExcep

if (_iriParsing && hasUnicode)
{
// In this scenario we need to parse the whole string
EnsureParseRemaining();
// In this scenario we need to parse the whole string
try
{
EnsureParseRemaining();
}
catch (UriFormatException ex)
{
e = ex;
return;
}
}
}
else
Expand Down Expand Up @@ -163,7 +171,15 @@ private void InitializeUri(ParsingError err, UriKind uriKind, out UriFormatExcep
if (_iriParsing && hasUnicode)
{
// In this scenario we need to parse the whole string
EnsureParseRemaining();
try
{
EnsureParseRemaining();
}
catch (UriFormatException ex)
{
e = ex;
return;
}
}
}
// will return from here
Expand All @@ -181,6 +197,11 @@ private void InitializeUri(ParsingError err, UriKind uriKind, out UriFormatExcep
// Iri'ze and then normalize relative uris
_string = EscapeUnescapeIri(_originalUnicodeString, 0, _originalUnicodeString.Length,
(UriComponents)0);
if (_string.Length > ushort.MaxValue)
{
err = ParsingError.SizeLimit;
return;
}
}
}
else
Expand Down
86 changes: 86 additions & 0 deletions src/System.Private.Uri/tests/FunctionalTests/IriTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,5 +599,91 @@ public void Iri_AllForbiddenDecompositions_NonIdnPropertiesOk(string scheme, str
Assert.Equal(host, uri.Authority);
Assert.Equal(scheme + "://" + host + "/", uri.AbsoluteUri);
}

// The behavior here is slightly complicated in order to preserve compat in as many
// cases as possible. There are two limits imposed on the length of URI strings.
// The first, 65519, is specified in the documentation and is one of the first checks
// enforced on a URI. This limit is not enforced after expansion.
private static int InitialLengthLimit = 65519;

// The second, 65535 (ushort.MaxValue) is only reachable via expansion as a result of
// percent encoding. Exceeding this value used to result in a hang, but now results in
// an exception.
private static int ExpandedLengthLimit = 65535;

// In order to maximize compat, we have to allow a gap between the two maximum
// values. A URI that starts below 65519 but expands to be in the range [65519,65535)
// would have worked before this change, and so should continue to work despite
// exceeding limit (1).
public static IEnumerable<Object[]> Iri_ExpandingContents_TooLong
{
get
{
// Validate a URI with an initial length less than InitialLengthLimit, and an expanded
// length that is greater than ExpandedLengthLimit.
// The total of len + const parts (15) + expanded unicode (2 * 9) after expansion should be
// just larger than ExpandedLengthLimit.
int len = ExpandedLengthLimit - 15 - (2 * 9) + 1;
yield return new object[] { @"test://" + new string('a', len) + new string('\uD800', 2) + "@8.8.8.8" }; // Userinfo
yield return new object[] { @"test://8.8.8.8?" + new string('a', len) + new string('\uD800', 2) }; // Query
yield return new object[] { @"test://8.8.8.8#" + new string('a', len) + new string('\uD800', 2) }; // Fragment
yield return new object[] { @"test://8.8.8.8/" + new string('a', len) + new string('\uD800', 2) }; // Path

// Generate a string whose total length is just less than InitialLengthLimit
// but whose content expands to be dramatically larger than ExpandedLengthLimit.
len = InitialLengthLimit - 15;
yield return new object[] { @"test://" + new string('\uD800', len) + "@8.8.8.8" }; // Userinfo
yield return new object[] { @"test://8.8.8.8?" + new string('\uD800', len) }; // Fragment
yield return new object[] { @"test://8.8.8.8#" + new string('\uD800', len) }; // Query
yield return new object[] { @"test://8.8.8.8/" + new string('\uD800', len) }; // Path

// Test the minimum length URI that will cause an expansion beyond ExpandedLengthLimit.
len = (ExpandedLengthLimit - 15) / 9 + 1;
yield return new object[] { @"test://" + new string('\uD800', len) + "@8.8.8.8" }; // Userinfo
yield return new object[] { @"test://8.8.8.8?" + new string('\uD800', len) }; // Fragment
yield return new object[] { @"test://8.8.8.8#" + new string('\uD800', len) }; // Query
yield return new object[] { @"test://8.8.8.8/" + new string('\uD800', len) }; // Path
}
}

[Theory]
[MemberData(nameof(Iri_ExpandingContents_TooLong))]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Disable until the .NET FX CI machines get the latest patches.")]
public static void Iri_ExpandingContents_ThrowsIfTooLong(string input)
{
Assert.Throws<System.UriFormatException>(() => { Uri itemUri = new Uri(input); });
Assert.False(Uri.TryCreate(input, UriKind.Absolute, out Uri itemUri2));
}

public static IEnumerable<Object[]> Iri_ExpandingContents_AllowedSize
{
get
{
// Validate a URI with an initial length less than InitialLengthLimit, and an expanded
// length that is greater than InitialLengthLimit but less than ExpandedLengthLimit.
// The total of len + const parts (15) + expanded unicode (2 * 9) after expansion should be
// exactly the ExpandedLengthLimit.
int len = ExpandedLengthLimit - 15 - (2 * 9);
yield return new object[] { @"test://" + new string('a', len) + new string('\uD800', 2) + "@8.8.8.8" }; // Userinfo
yield return new object[] { @"test://8.8.8.8?" + new string('a', len) + new string('\uD800', 2) }; // Query
yield return new object[] { @"test://8.8.8.8#" + new string('a', len) + new string('\uD800', 2) }; // Fragment
yield return new object[] { @"test://8.8.8.8/" + new string('a', len) + new string('\uD800', 2) }; // Path

// Validate the same behavior, but maximize the amount of expansion.
len = (ExpandedLengthLimit - 15) / 9;
yield return new object[] { @"test://" + new string('\uD800', len) + "@8.8.8.8" }; // Userinfo
yield return new object[] { @"test://8.8.8.8?" + new string('\uD800', len) }; // Fragment
yield return new object[] { @"test://8.8.8.8#" + new string('\uD800', len) }; // Query
yield return new object[] { @"test://8.8.8.8/" + new string('\uD800', len) }; // Path
}
}

[Theory]
[MemberData(nameof(Iri_ExpandingContents_AllowedSize))]
public static void Iri_ExpandingContents_DoesNotThrowIfSizeAllowed(string input)
{
Uri itemUri = new Uri(input);
Assert.True(Uri.TryCreate(input, UriKind.Absolute, out Uri itemUri2));
}
}
}