-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove more unsafe code from IPAddress parsing #121225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
This comment was marked as outdated.
This comment was marked as outdated.
bd7e21f to
79d0700
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@EgorBot -amd -arm -windows_intel using System;
using System.Net;
using BenchmarkDotNet.Attributes;
public class IPv4_u16_Benchmarks
{
public IEnumerable<string> Data() => [
"127.0.0.1",
"192.168.0.1",
"8.8.8.8",
"255.255.255.255"];
[Benchmark, ArgumentsSource(nameof(Data))]
public IPAddress Parse_IPv4_u16(string ip) => IPAddress.Parse(ip);
}
public class IPv4_u8_Benchmarks
{
public IEnumerable<byte[]> Data() => [
"127.0.0.1"u8.ToArray(),
"192.168.0.1"u8.ToArray(),
"8.8.8.8"u8.ToArray(),
"255.255.255.255"u8.ToArray()];
[Benchmark, ArgumentsSource(nameof(Data))]
public IPAddress Parse_IPv4_u8(byte[] ip) => IPAddress.Parse(ip);
}
public class IPv6_u16_Benchmarks
{
public IEnumerable<string> Data() => [
"::1",
"2001:db8::1",
"fe80::1ff:fe23:4567:890a",
"2001:4860:4860::8888"];
[Benchmark, ArgumentsSource(nameof(Data))]
public IPAddress Parse_IPv6_u16(string ip) => IPAddress.Parse(ip);
}
public class IPv6_u8_Benchmarks
{
public IEnumerable<byte[]> Data() => [
"::1"u8.ToArray(),
"2001:db8::1"u8.ToArray(),
"fe80::1ff:fe23:4567:890a"u8.ToArray(),
"2001:4860:4860::8888"u8.ToArray()];
[Benchmark, ArgumentsSource(nameof(Data))]
public IPAddress Parse_IPv6_u8(byte[] ip) => IPAddress.Parse(ip);
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the IPv4 and IPv6 address validation methods across the Uri and networking libraries to eliminate unsafe code and use ReadOnlySpan<T> instead of pointer-based operations. The key changes include:
- Converting pointer-based (
char*) validation methods to span-based implementations - Changing
ref int endparameters toout int endparameters with 0-based indexing - Removing
unsafeblocks andfixedstatements throughout the codebase
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.Uri/src/System/Uri.cs | Updated CheckHostName and CheckAuthorityHelper to use span-based IPv4/IPv6 helpers with adjusted index calculations |
| src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs | Converted InternalIsValid and IsValid from unsafe pointer methods to span-based methods |
| src/libraries/System.Private.Uri/src/System/IPv4AddressHelper.cs | Simplified ParseCanonicalName by removing unsafe code and using span slicing |
| src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs | Removed unsafe blocks from IsValid, TryParseIpv4, and TryParseIPv6 methods |
| src/libraries/Common/src/System/Net/IPv6AddressHelper.Common.cs | Converted IsValidStrict from pointer-based to span-based with adjusted indexing and removed sequenceLength = 0 assignment |
| src/libraries/Common/src/System/Net/IPv4AddressHelper.Common.cs | Refactored IsValid, IsValidCanonical, and ParseNonCanonical to use spans with 0-based indexing |
|
PTAL @stephentoub @MihaZupan It seems to slightly improve the performance a bit (see EgorBot/runtime-utils#534) due to a bit of tweaking, but mostly because of It seems like this code has a good test coverage, all off-by-one mistakes led to test failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
I also encountered a small out-of-bounds load in IPv6AddressHelper.InternalIsValid that was accessing string's null terminator most likely unintentionally
Nice catch
src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs
Outdated
Show resolved
Hide resolved
|
@MihuBot fuzz IPAddress |
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
|
/ba-g deadletter |
let's see what benchmarks think...