IPBlock not working with ip IPBlock::create('2a0d:7c40:1001::',48)

[cfinnestad]

returns 3261:3064:3a37:: as the first address not 2a0d:7c40:1001::

[cfinnestad]

$ip = '2a0d:7c40:1001::';
echo $ip ."\n";
$block = IPBlock::create($ip, '48');
echo $block->getFirstIp()->humanReadable()."\n";

results
2a0d:7c40:1001::
3261:3064:3a37::

[hwmatthewa]

Seems to be an issue with inet_ntop returning incorrectly with certain IPV6 addresses, causing it to enter this if statement and return an incorrect value as it should be entering this if statement instead to return the correct value.

[samuelwilliams]

It is because the string looks exactly like a packed IPv6 address, i.e.

$string = '2a0d:7c40:1001::';
$packed = inet_pton('3261:3064:3a37:6334:303a:3130:3031:3a3a');
echo $string === $packed; //Echos 1

I don't see a great way around these edge cases given how the IP::create() method tries to discern between different input types. The same can happen with IPv4:

echo (new PhpIP\IPv4("abcd"))->humanReadable(); // Echos "53.53.46.53"

PR #58 would mean that the IP::fromString() method will test for strings before packed addresses, but the trade off is that some packed addresses will be interpreted as strings; we would have the opposite problem...

//If PR 58 is merged
echo (new PhpIP\IPv4(inet_pton('3261:3064:3a37:6334:303a:3130:3031:3a3a')))->humanReadable(); // Echos "2a0d:7c40:1001::"

[rlanvin]

Thank you all for the investigation. As @samuelwilliams said, the problem is that PHP cannot distinguish a type binary string (result of inet_pton) and a type string (human readable) as they are both simply string for PHP. The constructor is getting confused, it thinks you are passing a in_addr representation (binary string) where in fact your are passing a human readable representation.

This bug cannot happen with IPv4, because the result of inet_pton for an IPv4 is 4 bytes, i.e. a 4 characters string. Therefore I can never be confused with an valid IP address version 4, since the shortest valid ("A.B.C.D") is a least 8 characters.

However for IPv6 the result of inet_pton is 16 bytes. Since IPv6 can be abbreviated, it's possible to have valid IPv6 that are 16 characters long (as in your example), and therefore, conflicts can happen. (At this point I facepalm for overlooking this fact when I originally wrote the lib...)

This is unfortunate, and I think this will require a breaking change in this library. For now the option that comes to my mind is to completely drop support for inet_* in the constructor for IPv6 given that it is impossible to reliably "guess" what the caller of the constructor is trying to do if we have a 16 character string that looks like a valid IP. We can replace it by:

1. Either nothing, requiring explicitly calling inet_ntop. It's easy enough for the user of the lib to write: new IPv6(inet_ntop('2a0d:7c40:1001::')) if the expected result is indeed 3261:3064:3a37::. However that makes an unnecessary conversion, since the lib then would do a inet_pton, but it might be the lesser evil.
2. Or an additional parameter to the constructor, indicating whether it's a binary string or not. I don't really like it, as it's bloating the API.
3. Or an explicit call to a factory method, e.g. IPv6::fromBinaryString() or something, to use specifically if you already have a binary string, to avoid double conversion.

What do you think?

[hwmatthewa]

I think that having explicit factory methods would probably be ideal. This way it is explicitly understood through the use of the factory methods what type of data is expected to be parsed. As a workaround we are currently doing what you noted in method 1 via a factory method we created.

[samuelwilliams]

The issue can occur in IPv4 as well if we were to try and pass a four digit integer string to the fromString method. i.e.

echo inet_ntop('1024'); //Echos "49.48.50.52"

Option 3 sounds like a good idea. Should we drop the binary string filter from the IP::fromString() method so that we are only looking at type strings? The method would then look like...

    private static function fromString(string $ip): \GMP
    {
        // valid IP string
        $filterFlag = constant('FILTER_FLAG_IPV'.static::IP_VERSION);
        if (filter_var($ip, FILTER_VALIDATE_IP, $filterFlag)) {
            $ip = inet_pton($ip);
            $hex = unpack('H*', $ip);

            return gmp_init($hex[1], 16);
        }

        // numeric string (decimal)
        if (ctype_digit($ip)) {
            $ip = gmp_init($ip, 10);
            if (!self::isValid($ip)) {
                throw new \InvalidArgumentException(sprintf('"%s" is not a valid decimal IPv%d address.', gmp_strval($ip), static::IP_VERSION));
            }

            return $ip;
        }

        throw new \InvalidArgumentException(sprintf('The string "%s" is not a valid IPv%d address.', $ip, static::IP_VERSION));
    }

[samuelwilliams]

PR #58 has new factory methods for creation of IP addresses from varying types. The new methods are:

• IP::newFromInteger(),
• IP::newFromFloat(),
• IP::newFromIpString(),
• IP::newFromBinaryString(),
• IP::newFromNumericString(),
• and IP::newFromGmp().

Tests have also been added to the IPv6 and IPv4 tests.

[rlanvin]

The issue can occur in IPv4 as well if we were to try and pass a four digit integer string to the fromString method. i.e.

It's not a issue, this is the expected behaviour. A four digit integer is unambiguously the in_addr representation. It cannot be confused with a human readable IPv4 ('1024' is not a valid human readable representation of an IPv4).

I'm having a look a the PR now, thank you for your effort.

[samuelwilliams]

@rlanvin I like the idea of leveraging the trait as it has cleaned up the code nicely. The pedant in me would add an abstract constructor to the trait, though it is not wholly necessary.

trait IPTrait
{
    abstract public function __construct($ip);
...
}

Otherwise I think this is a really good solution.

I have sent through PR #62 to fix a minor camel case typo.

[SilverFire]

Added possible alternative fix #63

[KlaasT]

Hi,

I'm running into the exact same problem and I need to get that running somehow. I want to use IPBlock to interpret if a IPv6 or IPv4 network is supplied and then do some additional calculations.

Is there any workaround to get the correct IPv6 displayed again?

Thanks very much.

[hwmatthewa]

@KlaasT

As a quick workaround we used the following:

IPBlock::create(IP::create(inet_pton($ip))->numeric(), $mask);