Runtime: IPAddress static fields are readonly, but their values aren't

[alfredmyers]

IPAddress static fields for Any, Broadcast, Local and None are marked readonly

https://github.com/dotnet/corefx/blob/b0b1452eaaa120dc8267ebbe5a93798f77ec5ed1/src/System.Net.Primitives/src/System/Net/IPAddress.cs#L20-L23

Because of that, one could take for granted that they would always correspond to respectively 0.0.0.0, 255.255.255.255, 127.0.0.1 and 255.255.255.255.

The thing is, it is possible to change their values through the obsoleted IPAddress.Address property such as in

using System;
using System.Net;

class Program
{
    static void Main()
    {
        Console.WriteLine("Well known IP addresses");

        Write("Any", IPAddress.Any);
        Write("Broadcast", IPAddress.Broadcast);
        Write("Loopback", IPAddress.Loopback);
        Write("None", IPAddress.None);

        Console.WriteLine();

        IPAddress.Any.Address = uint.MaxValue;
        IPAddress.Broadcast.Address = 0;
        IPAddress.Loopback.Address = IPAddress.Any.Address;


        Console.WriteLine("Well known IP addresses. Or are they?");

        Write("Any", IPAddress.Any);
        Write("Broadcast", IPAddress.Broadcast);
        Write("Loopback", IPAddress.Loopback);
        Write("None", IPAddress.None);

        Console.ReadKey();

        void Write(string name, IPAddress address)
        {
            Console.WriteLine($"{name}\t{address}");
        }
    }
}

Output:

Well known IP addresses
Any     0.0.0.0
Broadcast       255.255.255.255
Loopback        127.0.0.1
None    255.255.255.255

Well known IP addresses. Or are they?
Any     255.255.255.255
Broadcast       0.0.0.0
Loopback        255.255.255.255
None    0.0.0.0

Although unexpected, it's quite easy to spot what is being done in the sample code above. But given the scope of static members, a troubleshooter may spend a couple of hours before realizing that any piece of code running in the process (or the AppDomain in case of the .NET Framework), including external dependencies (which would have generated warnings during compilation for members or types marked with [ObsoleteAttribute]) could have changed the values of those fields.

The property has been obsoleted since .NET Framework 1.1 but is still lingering around and has been brought into .NET Core in 2.0 and into the .NET Standard 2.0 as well.

Given the pervasive use of these fields inside the .NET Core, it would be prudent to put safeguards in place to minimize the chance that their values are changed.

[karelz]

@alfredmyers what would you suggest to do?
It is part of .NET Standard due to its popularity (1%+ in API Port telemetry & other sources). It was added into .NET Standard, so we will have to live with it. It is marked obsolete, so new code should hopefully avoid using it.

The only thing that comes to mind is to check in set_Address if it is on one of the 'special' addresses and throw instead. That would add overhead and I am not convinced it is worth the trouble.

[alfredmyers]

@karelz by overhead did you mean checking against the values of each 'special' address or a simple bool check as was done in the pull request?

[karelz]

@alfredmyers your PR has overhead of extra bool. As I said earlier, I do not think it is worth the value which is very small IMO.

[alfredmyers]

@karelz Maybe the example code I used when opening the issue isn't getting my point across

Take for instance IPAddress.Any. It is used as a return value at https://source.dot.net/#System.Net.NetworkInformation/System/Net/NetworkInformation/SystemUnicastIPAddressInformation.cs,54

https://source.dot.net is intermittently broken today seemingly because it's being indexed, so if the link above doesn't work please try the on at https://referencesource.microsoft.com/#System/net/System/Net/NetworkInformation/SystemUnicastIPAddressInformation.cs,54

Following the references you can see that IPAddress.Any eventually leaks at a place that has no visible linkage to it's original reference.

using System;
using System.Net;
using System.Net.NetworkInformation;
using System.Net.Sockets;

namespace TestConsole2
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine($"IPAddress.Any = {IPAddress.Any}");

            var networkInterfaces = NetworkInterface.GetAllNetworkInterfaces();
            foreach (var networkInterface in networkInterfaces)
            {
                foreach (var unicastAddress in networkInterface.GetIPProperties().UnicastAddresses)
                {
                    unicastAddress.IPv4Mask.Address = uint.MaxValue - 1;  // <=======****
                }
            }
            Console.WriteLine($"IPAddress.Any = {IPAddress.Any}");
        }
    }
}

Results

IPAddress.Any = 0.0.0.0
IPAddress.Any = 254.255.255.255

[karelz]

@alfredmyers what is the real-world scenario where changing returned IPAddresses (your sample code) is a good/reasonable idea?

[alfredmyers]

@karelz That's the point. It's an awful idea. That's why someone leaving their job in bad terms could leave a little "gift" behind:

The code looks like it's operating on a IPv4Mask when it's really changing the value of IPAddress.Any.

unicastAddress.IPv4Mask.Address = 0xFFFF;

The thing is IPAddress.Any is used in several places:

• DNS
• Ping
• TcpListener, etc.

Continuing the example, TcpListener(int port) for instance would only listen to "255.255.0.0" instead of any port.

It will take a while until somebody figures out what's happening.

I understand, that the proposed solution is expensive and there's probably other ways to solve the issue, but did I at least get across my point that those IPAddress static fields can be leaked in ways that are hard to troubleshoot?

[alfredmyers]

@karelz , @stephentoub
Given the code guards already in place separating the code paths between IPv4 and IPv6, an Array.Empty<uint>() could be stored in _numbers as a signal for readonly.
No extra boolean, no increase in size.

[roji]

Remove label needs more info?

[roji]

Any plans for carrying this over into .NET Standard 2.1?

[stephentoub]

Any plans for carrying this over into .NET Standard 2.1?

@roji, what would that mean? This didn't affect public surface area signatures.

[roji]

I specifically have a case of breakage, where a library is comparing a type against typeof(IPAddress), and the case of IPAddress.Any fails in netcoreapp3.0 but not others (checking instead whether that type is assignable is undesirable for perf reasons).

I don't know whether this kind of thing is covered by any sort of formal backwards compatibility guarantees, or whether .NET Standard says anything about it. For example, the standard could make mandate that the specific type of IPAddress.Any would be the same across implementations, so that libraries targeting it would be able to assume that.

(I'm assuming the answer is that no such coherency guarantees are made, but asking just in case).

[stephentoub]

I see. Even in .NET Framework where back compat was even more stringent, we still generally considered it acceptable to return a derived type where a base was specified, to throw a more derived exception, to change the concrete type we return for an interface implementation, etc.

[stephentoub]

I specifically have a case of breakage

How big a deal is it?

Unfortunately pretty much any bug fix can break something :)

[roji]

It's definitely not a big deal - I don't think you should refrain from returning a more derived type or similar.

What could have been helpful, is to make sure the same type gets returned across a specific version of the .NET Standard, so that a library targeting it would have more predictable results. Thinking more about that, though, since later implementations still implement earlier versions of the standard, this wouldn't be much of a guarantee either...

Otherwise the type being internal doesn't make things any easier in my specific library, since reflection needs to be used.

The more I think about it, the less I think I have an actual ask here, you should probably feel free to ignore :)