Skip to content

Conversation

@thomascellerier
Copy link
Contributor

@thomascellerier thomascellerier commented Jan 17, 2022

IPv*Network and IPv*Interface constructors accept a 2-tuple of (address description, netmask) as the address parameter.
When the tuple-based address is used errors are not propagated correctly through the ipaddress.ip_* helper because of the %-formatting now expecting several arguments:

In [7]: ipaddress.ip_network(("192.168.100.0", "fooo"))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-7-7fc0bff07012> in <module>
----> 1 ipaddress.ip_network(("192.168.100.0", "fooo"))

/usr/lib/python3.8/ipaddress.py in ip_network(address, strict)
     81         pass
     82
---> 83     raise ValueError('%r does not appear to be an IPv4 or IPv6 network' %
     84                      address)
     85

TypeError: not all arguments converted during string formatting

Compared to:

In [8]: ipaddress.IPv4Network(("192.168.100.0", "foo"))
---------------------------------------------------------------------------
NetmaskValueError                         Traceback (most recent call last)
<ipython-input-8-79078758e653> in <module>
----> 1 ipaddress.IPv4Network(("192.168.100.0", "foo"))

/usr/lib/python3.8/ipaddress.py in __init__(self, address, strict)
   1453
   1454         self.network_address = IPv4Address(addr)
-> 1455         self.netmask, self._prefixlen = self._make_netmask(mask)
   1456         packed = int(self.network_address)
   1457         if packed & int(self.netmask) != packed:

/usr/lib/python3.8/ipaddress.py in _make_netmask(cls, arg)
   1118                     # Check for a netmask or hostmask in dotted-quad form.
   1119                     # This may raise NetmaskValueError.
-> 1120                     prefixlen = cls._prefix_from_ip_string(arg)
   1121             netmask = IPv4Address(cls._ip_int_from_prefix(prefixlen))
   1122             cls._netmask_cache[arg] = netmask, prefixlen

/usr/lib/python3.8/ipaddress.py in _prefix_from_ip_string(cls, ip_str)
    516             ip_int = cls._ip_int_from_string(ip_str)
    517         except AddressValueError:
--> 518             cls._report_invalid_netmask(ip_str)
    519
    520         # Try matching a netmask (this would be /1*0*/ as a bitwise regexp).

/usr/lib/python3.8/ipaddress.py in _report_invalid_netmask(cls, netmask_str)
    472     def _report_invalid_netmask(cls, netmask_str):
    473         msg = '%r is not a valid netmask' % netmask_str
--> 474         raise NetmaskValueError(msg) from None
    475
    476     @classmethod

NetmaskValueError: 'foo' is not a valid netmask

Use an f-string to make sure the error is always properly formatted.

https://bugs.python.org/issue46415

`IPv*Network` and `IPv*Interface` constructors accept a 2-tuple of
(address description, netmask) as the address parameter.
When the tuple-based address is used errors are not propagated
correctly through the `ipaddress.ip_*` helper because of the %-formatting now expecting several arguments:

	In [7]: ipaddress.ip_network(("192.168.100.0", "fooo"))
	---------------------------------------------------------------------------
	TypeError                                 Traceback (most recent call last)
	<ipython-input-7-7fc0bff07012> in <module>
	----> 1 ipaddress.ip_network(("192.168.100.0", "fooo"))

	/usr/lib/python3.8/ipaddress.py in ip_network(address, strict)
	     81         pass
	     82
	---> 83     raise ValueError('%r does not appear to be an IPv4 or IPv6 network' %
	     84                      address)
	     85

	TypeError: not all arguments converted during string formatting

Compared to:

	In [8]: ipaddress.IPv4Network(("192.168.100.0", "foo"))
	---------------------------------------------------------------------------
	NetmaskValueError                         Traceback (most recent call last)
	<ipython-input-8-79078758e653> in <module>
	----> 1 ipaddress.IPv4Network(("192.168.100.0", "foo"))

	/usr/lib/python3.8/ipaddress.py in __init__(self, address, strict)
	   1453
	   1454         self.network_address = IPv4Address(addr)
	-> 1455         self.netmask, self._prefixlen = self._make_netmask(mask)
	   1456         packed = int(self.network_address)
	   1457         if packed & int(self.netmask) != packed:

	/usr/lib/python3.8/ipaddress.py in _make_netmask(cls, arg)
	   1118                     # Check for a netmask or hostmask in dotted-quad form.
	   1119                     # This may raise NetmaskValueError.
	-> 1120                     prefixlen = cls._prefix_from_ip_string(arg)
	   1121             netmask = IPv4Address(cls._ip_int_from_prefix(prefixlen))
	   1122             cls._netmask_cache[arg] = netmask, prefixlen

	/usr/lib/python3.8/ipaddress.py in _prefix_from_ip_string(cls, ip_str)
	    516             ip_int = cls._ip_int_from_string(ip_str)
	    517         except AddressValueError:
	--> 518             cls._report_invalid_netmask(ip_str)
	    519
	    520         # Try matching a netmask (this would be /1*0*/ as a bitwise regexp).

	/usr/lib/python3.8/ipaddress.py in _report_invalid_netmask(cls, netmask_str)
	    472     def _report_invalid_netmask(cls, netmask_str):
	    473         msg = '%r is not a valid netmask' % netmask_str
	--> 474         raise NetmaskValueError(msg) from None
	    475
	    476     @classmethod

	NetmaskValueError: 'foo' is not a valid netmask

Use an f-string to make sure the error is always properly formatted.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@thomascellerier

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! This is a classic bug with % formatting.

There are a lot of other instances of % formatting in this file. Most are safe, because they explicitly pass tuples or first check that the argument is some non-tuple type, but I found two others that should probably be fixed:

>>> ipaddress.IPv4Address(("x/", "y"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jelle/py/cpython/Lib/ipaddress.py", line 1307, in __init__
    raise AddressValueError("Unexpected '/' in %r" % address)
                            ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
TypeError: not all arguments converted during string formatting
>>> ipaddress.IPv6Address(("x/", "y"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jelle/py/cpython/Lib/ipaddress.py", line 1916, in __init__
    raise AddressValueError("Unexpected '/' in %r" % address)
                            ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
TypeError: not all arguments converted during string formatting

@thomascellerier
Copy link
Contributor Author

Ah indeed!

I pushed a fix for those 2 and one additional case I found while going through the module.

@JelleZijlstra
Copy link
Member

I just found #30087 which fixes the same thing (though not the additional cases we found here, and without tests).

@thomascellerier
Copy link
Contributor Author

Hmm OK I see. Should I close this PR? Add more tests for the other cases?

@JelleZijlstra
Copy link
Member

That's up to the core dev doing the merge I suppose. I'd leave this open until a core dev comes along with a decision.

@hugovk
Copy link
Member

hugovk commented Apr 9, 2022

That's up to the core dev doing the merge I suppose. I'd leave this open until a core dev comes along with a decision.

@JelleZijlstra you're a core dev now, would you like to come along with a decision? :)

@ghost
Copy link

ghost commented May 2, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@JelleZijlstra
Copy link
Member

I'd like to merge this one, but @thomascellerier will have to resign the CLA with the new process, sorry for the delay!

@JelleZijlstra JelleZijlstra self-assigned this May 2, 2022
@thomascellerier
Copy link
Contributor Author

Done!

@miss-islington
Copy link
Contributor

Thanks @thomascellerier for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-92225 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 3, 2022
@bedevere-bot
Copy link

GH-92226 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 3, 2022
…rk,interface} helper functions (pythonGH-30642)

`IPv*Network` and `IPv*Interface` constructors accept a 2-tuple of
(address description, netmask) as the address parameter.
When the tuple-based address is used errors are not propagated
correctly through the `ipaddress.ip_*` helper because of the %-formatting now expecting several arguments:

	In [7]: ipaddress.ip_network(("192.168.100.0", "fooo"))
        ...
	TypeError: not all arguments converted during string formatting

Compared to:

	In [8]: ipaddress.IPv4Network(("192.168.100.0", "foo"))
        ...
	NetmaskValueError: 'foo' is not a valid netmask

Use an f-string to make sure the error is always properly formatted.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 52dc9c3)

Co-authored-by: Thomas Cellerier <thomascellerier@gmail.com>
@JelleZijlstra
Copy link
Member

I wasn't sure about whether to backport this, but I think the old behavior is buggy and worth fixing in the other branches. On the other hand, we do change the exception class raised, which could break users' code.

miss-islington added a commit that referenced this pull request May 3, 2022
…rk,interface} helper functions (GH-30642)

`IPv*Network` and `IPv*Interface` constructors accept a 2-tuple of
(address description, netmask) as the address parameter.
When the tuple-based address is used errors are not propagated
correctly through the `ipaddress.ip_*` helper because of the %-formatting now expecting several arguments:

	In [7]: ipaddress.ip_network(("192.168.100.0", "fooo"))
        ...
	TypeError: not all arguments converted during string formatting

Compared to:

	In [8]: ipaddress.IPv4Network(("192.168.100.0", "foo"))
        ...
	NetmaskValueError: 'foo' is not a valid netmask

Use an f-string to make sure the error is always properly formatted.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 52dc9c3)

Co-authored-by: Thomas Cellerier <thomascellerier@gmail.com>
miss-islington added a commit that referenced this pull request May 3, 2022
…rk,interface} helper functions (GH-30642)

`IPv*Network` and `IPv*Interface` constructors accept a 2-tuple of
(address description, netmask) as the address parameter.
When the tuple-based address is used errors are not propagated
correctly through the `ipaddress.ip_*` helper because of the %-formatting now expecting several arguments:

	In [7]: ipaddress.ip_network(("192.168.100.0", "fooo"))
        ...
	TypeError: not all arguments converted during string formatting

Compared to:

	In [8]: ipaddress.IPv4Network(("192.168.100.0", "foo"))
        ...
	NetmaskValueError: 'foo' is not a valid netmask

Use an f-string to make sure the error is always properly formatted.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 52dc9c3)

Co-authored-by: Thomas Cellerier <thomascellerier@gmail.com>
@thomascellerier thomascellerier deleted the ip-address-tuple-value-error branch May 5, 2022 08:44
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
…rk,interface} helper functions (pythonGH-30642)

`IPv*Network` and `IPv*Interface` constructors accept a 2-tuple of
(address description, netmask) as the address parameter.
When the tuple-based address is used errors are not propagated
correctly through the `ipaddress.ip_*` helper because of the %-formatting now expecting several arguments:

	In [7]: ipaddress.ip_network(("192.168.100.0", "fooo"))
        ...
	TypeError: not all arguments converted during string formatting

Compared to:

	In [8]: ipaddress.IPv4Network(("192.168.100.0", "foo"))
        ...
	NetmaskValueError: 'foo' is not a valid netmask

Use an f-string to make sure the error is always properly formatted.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit 52dc9c3)

Co-authored-by: Thomas Cellerier <thomascellerier@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants