from Paul Marks:

The previous code checks each octet separately, and then checks if
the sequence of octets is nonincreasing.  This is insufficient, because
it allows nonsensical inputs like "255.128.128.0".

The previous code also only partially handles hostmasks (inverted netmasks).
"0.255.255.255" becomes /8, but 0.15.255.255 is rejected.

The solution is to disregard octets, and interpret the address as a
single sequence of bits.

Note that this could be trivially extended to support IPv6 netmasks and
hostmasks, but those are non-standard.

I've also made the CIDR parser more strict, so it rejects anything that's
not not a decimal digit, like /+24

Added:
- _prefix_from_prefix_string: Parse a number, with bounds checking.
- _prefix_from_ip_string: Parse IPv4 netmask/hostmask string.

Improved:
- _prefix_from_ip_int: Only accept inputs with the bit sequence /1*0*/.

Removed:
- _ip_string_from_prefix
- _is_hostmask
- _is_valid_netmask

2 files changed, 147 insertions, 124 deletions

diff --git a/trunk/ipaddr.py b/trunk/ipaddr.py
index 30768d9..c9aee63 100644
--- a/trunk/ipaddr.py
+++ b/trunk/ipaddr.py
@@ -839,8 +839,8 @@ class _BaseNet(_IPAddrBase):
         """
         return (self._version, self.network, self.netmask)
 
-    def _ip_int_from_prefix(self, prefixlen=None):
-        """Turn the prefix length netmask into a int for comparison.
+    def _ip_int_from_prefix(self, prefixlen):
+        """Turn the prefix length into a bitwise netmask.
 
         Args:
             prefixlen: An integer, the prefix length.
@@ -849,42 +849,90 @@ class _BaseNet(_IPAddrBase):
             An integer.
 
         """
-        if not prefixlen and prefixlen != 0:
-            prefixlen = self._prefixlen
         return self._ALL_ONES ^ (self._ALL_ONES >> prefixlen)
 
-    def _prefix_from_ip_int(self, ip_int, mask=32):
-        """Return prefix length from the decimal netmask.
+    def _prefix_from_ip_int(self, ip_int):
+        """Return prefix length from a bitwise netmask.
 
         Args:
-            ip_int: An integer, the IP address.
-            mask: The netmask.  Defaults to 32.
+            ip_int: An integer, the netmask in expanded bitwise format.
 
         Returns:
             An integer, the prefix length.
 
+        Raises:
+            NetmaskValueError: If the input is not a valid netmask.
+
         """
-        while mask:
-            if ip_int & 1 == 1:
+        prefixlen = self._max_prefixlen
+        while prefixlen:
+            if ip_int & 1:
                 break
             ip_int >>= 1
-            mask -= 1
+            prefixlen -= 1
+
+        if ip_int == (1 << prefixlen) - 1:
+            return prefixlen
+        else:
+            raise NetmaskValueError('Bit pattern does not match /1*0*/')
+
+    def _prefix_from_prefix_string(self, prefixlen_str):
+        """Turn a prefix length string into an integer.
+
+        Args:
+            prefixlen_str: A decimal string containing the prefix length.
+
+        Returns:
+            The prefix length as an integer.
 
-        return mask
+        Raises:
+            NetmaskValueError: If the input is malformed or out of range.
+
+        """
+        try:
+            if not _BaseV4._DECIMAL_DIGITS.issuperset(prefixlen_str):
+                raise ValueError
+            prefixlen = int(prefixlen_str)
+            if not (0 <= prefixlen <= self._max_prefixlen):
+               raise ValueError
+        except ValueError:
+            raise NetmaskValueError('%s is not a valid prefix length' %
+                                    prefixlen_str)
+        return prefixlen
 
-    def _ip_string_from_prefix(self, prefixlen=None):
-        """Turn a prefix length into a dotted decimal string.
+    def _prefix_from_ip_string(self, ip_str):
+        """Turn a netmask/hostmask string into a prefix length.
 
         Args:
-            prefixlen: An integer, the netmask prefix length.
+            ip_str: A netmask or hostmask, formatted as an IP address.
 
         Returns:
-            A string, the dotted decimal netmask string.
+            The prefix length as an integer.
+
+        Raises:
+            NetmaskValueError: If the input is not a netmask or hostmask.
 
         """
-        if not prefixlen:
-            prefixlen = self._prefixlen
-        return self._string_from_ip_int(self._ip_int_from_prefix(prefixlen))
+        # Parse the netmask/hostmask like an IP address.
+        try:
+            ip_int = self._ip_int_from_string(ip_str)
+        except AddressValueError:
+            raise NetmaskValueError('%s is not a valid netmask' % ip_str)
+
+        # Try matching a netmask (this would be /1*0*/ as a bitwise regexp).
+        # Note that the two ambiguous cases (all-ones and all-zeroes) are
+        # treated as netmasks.
+        try:
+            return self._prefix_from_ip_int(ip_int)
+        except NetmaskValueError:
+            pass
+
+        # Invert the bits, and try matching a /0+1+/ hostmask instead.
+        ip_int ^= self._ALL_ONES
+        try:
+            return self._prefix_from_ip_int(ip_int)
+        except NetmaskValueError:
+            raise NetmaskValueError('%s is not a valid netmask' % ip_str)
 
     def iter_subnets(self, prefixlen_diff=1, new_prefix=None):
         """The subnets which join to make the current subnet.
@@ -927,7 +975,7 @@ class _BaseNet(_IPAddrBase):
             raise ValueError('prefix length diff must be > 0')
         new_prefixlen = self._prefixlen + prefixlen_diff
 
-        if not self._is_valid_netmask(str(new_prefixlen)):
+        if new_prefixlen > self._max_prefixlen:
             raise ValueError(
                 'prefix length diff %d is invalid for netblock %s' % (
                     new_prefixlen, str(self)))
@@ -1227,9 +1275,6 @@ class IPv4Network(_BaseV4, _BaseNet):
 
     """
 
-    # the valid octets for host and netmasks. only useful for IPv4.
-    _valid_mask_octets = set((255, 254, 252, 248, 240, 224, 192, 128, 0))
-
     def __init__(self, address, strict=False):
         """Instantiate a new IPv4 network object.
 
@@ -1292,31 +1337,18 @@ class IPv4Network(_BaseV4, _BaseNet):
         self.ip = IPv4Address(self._ip)
 
         if len(addr) == 2:
-            mask = addr[1].split('.')
-            if len(mask) == 4:
-                # We have dotted decimal netmask.
-                if self._is_valid_netmask(addr[1]):
-                    self.netmask = IPv4Address(self._ip_int_from_string(
-                            addr[1]))
-                elif self._is_hostmask(addr[1]):
-                    self.netmask = IPv4Address(
-                        self._ip_int_from_string(addr[1]) ^ self._ALL_ONES)
-                else:
-                    raise NetmaskValueError('%s is not a valid netmask'
-                                                     % addr[1])
-
-                self._prefixlen = self._prefix_from_ip_int(int(self.netmask))
-            else:
-                # We have a netmask in prefix length form.
-                if not self._is_valid_netmask(addr[1]):
-                    raise NetmaskValueError(addr[1])
-                self._prefixlen = int(addr[1])
-                self.netmask = IPv4Address(self._ip_int_from_prefix(
-                    self._prefixlen))
+            try:
+                # Check for a netmask in prefix length form.
+                self._prefixlen = self._prefix_from_prefix_string(addr[1])
+            except NetmaskValueError:
+                # Check for a netmask or hostmask in dotted-quad form.
+                # This may raise NetmaskValueError.
+                self._prefixlen = self._prefix_from_ip_string(addr[1])
         else:
             self._prefixlen = self._max_prefixlen
-            self.netmask = IPv4Address(self._ip_int_from_prefix(
-                self._prefixlen))
+
+        self.netmask = IPv4Address(self._ip_int_from_prefix(self._prefixlen))
+
         if strict:
             if self.ip != self.network:
                 raise ValueError('%s has host bits set' %
@@ -1324,53 +1356,6 @@ class IPv4Network(_BaseV4, _BaseNet):
         if self._prefixlen == (self._max_prefixlen - 1):
             self.iterhosts = self.__iter__
 
-    def _is_hostmask(self, ip_str):
-        """Test if the IP string is a hostmask (rather than a netmask).
-
-        Args:
-            ip_str: A string, the potential hostmask.
-
-        Returns:
-            A boolean, True if the IP string is a hostmask.
-
-        """
-        bits = ip_str.split('.')
-        try:
-            parts = [int(x) for x in bits if int(x) in self._valid_mask_octets]
-        except ValueError:
-            return False
-        if len(parts) != len(bits):
-            return False
-        if parts[0] < parts[-1]:
-            return True
-        return False
-
-    def _is_valid_netmask(self, netmask):
-        """Verify that the netmask is valid.
-
-        Args:
-            netmask: A string, either a prefix or dotted decimal
-              netmask.
-
-        Returns:
-            A boolean, True if the prefix represents a valid IPv4
-            netmask.
-
-        """
-        mask = netmask.split('.')
-        if len(mask) == 4:
-            if [x for x in mask if int(x) not in self._valid_mask_octets]:
-                return False
-            if [y for idx, y in enumerate(mask) if idx > 0 and
-                y > mask[idx - 1]]:
-                return False
-            return True
-        try:
-            netmask = int(netmask)
-        except ValueError:
-            return False
-        return 0 <= netmask <= self._max_prefixlen
-
     # backwards compatibility
     IsRFC1918 = lambda self: self.is_private
     IsMulticast = lambda self: self.is_multicast
@@ -1861,10 +1846,8 @@ class IPv6Network(_BaseV6, _BaseNet):
         self.ip = IPv6Address(self._ip)
 
         if len(addr) == 2:
-            if self._is_valid_netmask(addr[1]):
-                self._prefixlen = int(addr[1])
-            else:
-                raise NetmaskValueError(addr[1])
+            # This may raise NetmaskValueError
+            self._prefixlen = self._prefix_from_prefix_string(addr[1])
         else:
             self._prefixlen = self._max_prefixlen
 
@@ -1877,23 +1860,6 @@ class IPv6Network(_BaseV6, _BaseNet):
         if self._prefixlen == (self._max_prefixlen - 1):
             self.iterhosts = self.__iter__
 
-    def _is_valid_netmask(self, prefixlen):
-        """Verify that the netmask/prefixlen is valid.
-
-        Args:
-            prefixlen: A string, the netmask in prefix length format.
-
-        Returns:
-            A boolean, True if the prefix represents a valid IPv6
-            netmask.
-
-        """
-        try:
-            prefixlen = int(prefixlen)
-        except ValueError:
-            return False
-        return 0 <= prefixlen <= self._max_prefixlen
-
     @property
     def with_netmask(self):
         return self.with_prefixlen
diff --git a/trunk/ipaddr_test.py b/trunk/ipaddr_test.py
index 5f50961..89a6893 100755
--- a/trunk/ipaddr_test.py
+++ b/trunk/ipaddr_test.py
@@ -158,7 +158,6 @@ class IpaddrUnitTest(unittest.TestCase):
         self.assertRaises(ipaddr.AddressValueError,
                           ipaddr.IPv4Address(1)._ip_int_from_string,
                           '1.a.2.3')
-        self.assertEqual(False, ipaddr.IPv4Network(1)._is_hostmask('1.a.2.3'))
 
     def testGetNetwork(self):
         self.assertEqual(int(self.ipv4.network), 16909056)
@@ -277,11 +276,11 @@ class IpaddrUnitTest(unittest.TestCase):
     def testZeroNetmask(self):
         ipv4_zero_netmask = ipaddr.IPv4Network('1.2.3.4/0')
         self.assertEqual(int(ipv4_zero_netmask.netmask), 0)
-        self.assertTrue(ipv4_zero_netmask._is_valid_netmask(str(0)))
+        self.assertEqual(ipv4_zero_netmask._prefix_from_prefix_string('0'), 0)
 
         ipv6_zero_netmask = ipaddr.IPv6Network('::1/0')
         self.assertEqual(int(ipv6_zero_netmask.netmask), 0)
-        self.assertTrue(ipv6_zero_netmask._is_valid_netmask(str(0)))
+        self.assertEqual(ipv6_zero_netmask._prefix_from_prefix_string('0'), 0)
 
     def testGetBroadcast(self):
         self.assertEqual(int(self.ipv4.broadcast), 16909311L)
@@ -435,20 +434,83 @@ class IpaddrUnitTest(unittest.TestCase):
         self.assertRaises(ipaddr.AddressValueError,
                           ipaddr.IPv6Network, '10/8')
 
-
-    def testBadNetMask(self):
+    def testGoodNetmaskIPv4(self):
+        self.assertEqual(str(ipaddr.IPv4Network('192.0.2.0/255.255.255.0')),
+                         '192.0.2.0/24')
+        for i in range(0, 33):
+            # Generate and re-parse the CIDR format (trivial).
+            net_str = '0.0.0.0/%d' % i
+            net = ipaddr.IPv4Network(net_str)
+            self.assertEqual(str(net), net_str)
+
+            # Generate and re-parse the expanded netmask.
+            self.assertEqual(
+                str(ipaddr.IPv4Network('0.0.0.0/%s' % net.netmask)),
+                net_str)
+
+            # Zero prefix is treated as decimal.
+            self.assertEqual(str(ipaddr.IPv4Network('0.0.0.0/0%d' % i)),
+                             net_str)
+
+            # Generate and re-parse the expanded hostmask.  The ambiguous cases
+            # (/0 and /32) are treated as netmasks.
+            if i in (32, 0):
+                net_str = '0.0.0.0/%d' % (32 - i)
+            self.assertEqual(
+                str(ipaddr.IPv4Network('0.0.0.0/%s' % net.hostmask)),
+                net_str)
+
+    def testGoodNetmaskIPv6(self):
+        # We only support CIDR for IPv6, because expanded netmasks are not
+        # standard notation.
+        self.assertEqual(str(ipaddr.IPv6Network('2001:db8::/32')),
+                         '2001:db8::/32')
+        for i in range(0, 129):
+            # Generate and re-parse the CIDR format (trivial).
+            net_str = '::/%d' % i
+            self.assertEqual(str(ipaddr.IPv6Network(net_str)), net_str)
+
+            # Zero prefix is treated as decimal.
+            self.assertEqual(str(ipaddr.IPv6Network('::/0%d' % i)), net_str)
+
+    def testBadNetmask(self):
         self.assertRaises(ipaddr.NetmaskValueError,
                           ipaddr.IPv4Network, '1.2.3.4/')
         self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.2.3.4/-1')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.2.3.4/+1')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.2.3.4/0x1')
+        self.assertRaises(ipaddr.NetmaskValueError,
                           ipaddr.IPv4Network, '1.2.3.4/33')
         self.assertRaises(ipaddr.NetmaskValueError,
                           ipaddr.IPv4Network, '1.2.3.4/254.254.255.256')
         self.assertRaises(ipaddr.NetmaskValueError,
                           ipaddr.IPv4Network, '1.1.1.1/240.255.0.0')
         self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.1.1.1/255.254.128.0')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.1.1.1/0.1.127.255')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.2.3.4/1.a.2.3')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv4Network, '1.1.1.1/::')
+        self.assertRaises(ipaddr.NetmaskValueError,
                           ipaddr.IPv6Network, '::1/')
         self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv6Network, '::1/-1')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv6Network, '::1/+1')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv6Network, '::1/0x1')
+        self.assertRaises(ipaddr.NetmaskValueError,
                           ipaddr.IPv6Network, '::1/129')
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv6Network, '::1/1.2.3.4')
+        # IPv6 expanded form is currently not supported.
+        self.assertRaises(ipaddr.NetmaskValueError,
+                          ipaddr.IPv6Network, '::/::')
 
     def testNth(self):
         self.assertEqual(str(self.ipv4[5]), '1.2.3.5')
@@ -762,11 +824,6 @@ class IpaddrUnitTest(unittest.TestCase):
         self.assertEqual(ipaddr.IPv6Network('::1:0:0:0:0').packed,
                          _cb('\x00' * 6 + '\x00\x01' + '\x00' * 8))
 
-    def testIpStrFromPrefixlen(self):
-        ipv4 = ipaddr.IPv4Network('1.2.3.4/24')
-        self.assertEqual(ipv4._ip_string_from_prefix(), '255.255.255.0')
-        self.assertEqual(ipv4._ip_string_from_prefix(28), '255.255.255.240')
-
     def testIpType(self):
         ipv4net = ipaddr.IPNetwork('1.2.3.4')
         ipv4addr = ipaddr.IPAddress('1.2.3.4')