Remove more unsafe code from IPAddress parsing#121225
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.
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. |
MihaZupan
left a comment
There was a problem hiding this comment.
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...