Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipaddress subnet slicing iterator malfunction #71870

Closed
tklausmann mannequin opened this issue Aug 4, 2016 · 16 comments
Closed

ipaddress subnet slicing iterator malfunction #71870

tklausmann mannequin opened this issue Aug 4, 2016 · 16 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@tklausmann
Copy link
Mannequin

tklausmann mannequin commented Aug 4, 2016

BPO 27683
Nosy @ncoghlan, @pitrou, @bitdancer, @serhiy-storchaka, @zhangyangyu, @Vgr255, @miss-islington
PRs
  • bpo-27683: Fix a regression for ipaddress networks that hosts() result #6016
  • [3.7] bpo-27683: Fix a regression for host() of ipaddress network objects (GH-6016) #6168
  • [3.6] bpo-27683: Fix a regression for host() of ipaddress network objects (GH-6016) #6169
  • Files
  • ipaddress_iter_fix.patch
  • issue27683.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-03-21.01:58:25.175>
    created_at = <Date 2016-08-04.13:07:37.189>
    labels = ['3.7', '3.8', 'type-bug', 'library']
    title = 'ipaddress subnet slicing iterator malfunction'
    updated_at = <Date 2018-03-21.01:58:25.118>
    user = 'https://bugs.python.org/tklausmann'

    bugs.python.org fields:

    activity = <Date 2018-03-21.01:58:25.118>
    actor = 'xiang.zhang'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-03-21.01:58:25.175>
    closer = 'xiang.zhang'
    components = ['Library (Lib)']
    creation = <Date 2016-08-04.13:07:37.189>
    creator = 'tklausmann'
    dependencies = []
    files = ['44008', '44193']
    hgrepos = []
    issue_num = 27683
    keywords = ['patch', '3.5regression']
    message_count = 16.0
    messages = ['271972', '271974', '271976', '271979', '271981', '271982', '271983', '272018', '272031', '273419', '274756', '279609', '279866', '314175', '314178', '314181']
    nosy_count = 11.0
    nosy_names = ['ncoghlan', 'pitrou', 'kormat', 'pmoody', 'r.david.murray', 'era', 'serhiy.storchaka', 'xiang.zhang', 'abarry', 'tklausmann', 'miss-islington']
    pr_nums = ['6016', '6168', '6169']
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27683'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @tklausmann
    Copy link
    Mannequin Author

    tklausmann mannequin commented Aug 4, 2016

    Between Python 3.4 (3.4.5 I've tested with) and Python 3.5 (3.5.2), the behavior of ipaddress's subnet() method changed.

    This code:

    $ cat iat.py
    import ipaddress
    p = ipaddress.IPv4Network("172.0.0.4/30")
    subnets = p.subnets(new_prefix=31)
    n = next(subnets)
    
    print(list(n.hosts()))

    has two different outcomes on 3.4 vs. 3.5:

    $ python3.4 iat.py 
    [IPv4Address('172.0.0.4'), IPv4Address('172.0.0.5')]
    $ python3.5 iat.py 
    []

    AIUI, the 3.4 behavior is the correct one (or the docs need to be fixed).

    @tklausmann tklausmann mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 4, 2016
    @bitdancer
    Copy link
    Member

    If someone wants to bisect to find out which changeset introduced the behavior change that would be helpful.

    @serhiy-storchaka
    Copy link
    Member

    The change was introduced in bpo-21487.

    @tklausmann
    Copy link
    Mannequin Author

    tklausmann mannequin commented Aug 4, 2016

    I just bisected it and the specific commit is this one:

    https://hg.python.org/cpython/rev/2711677cf874

    @bitdancer
    Copy link
    Member

    Since this is a regression, I'm going to claim this has higher than normal priority, for whatever good that will do :)

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Aug 4, 2016

    Here's a patch that fixes this bug, test included. I made this against the 3.5 branch, but should apply cleanly on default.

    @kormat
    Copy link
    Mannequin

    kormat mannequin commented Aug 4, 2016

    The bug appears to be in the new form of the constructor. Here's a more minimal reproduction:

    In python3.5:
    >>> list(ipaddress.IPv4Network(("127.0.0.4", 31)).hosts())
    []
    
    In python3.4
    >>> list(ipaddress.IPv4Network("127.0.0.4/31").hosts())
    [IPv4Address('127.0.0.4'), IPv4Address('127.0.0.5')]

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Aug 5, 2016

    As Stephen notes, the underlying problem appears to be a behavioural difference between two theoretically equivalent ways of defining a network:

    >>> list(ipaddress.IPv4Network(('127.0.0.4', 31)).hosts())
    []
    >>> list(ipaddress.IPv4Network(('127.0.0.4/31')).hosts())
    [IPv4Address('127.0.0.4'), IPv4Address('127.0.0.5')]

    Now, the first case is the documented behaviour: hosts() is *supposed to* exclude the network and broadcast address, and those are the only addresses in a /31.

    If you want to iterate over all the *addresses* in a network (including the network and broadcast addresses) then you need to iterate over the network object directly:

    >>> list(ipaddress.IPv4Network(('127.0.0.4', 31)))
    [IPv4Address('127.0.0.4'), IPv4Address('127.0.0.5')]
    >>> list(ipaddress.IPv4Network(('127.0.0.4/31')))
    [IPv4Address('127.0.0.4'), IPv4Address('127.0.0.5')]

    However, as Emanuel found when writing his patch, there's currently an undocumented special case for /31 networks: the definition of "hosts" is *implicitly changed* for such instances to include the nominal network and broadcast address (by setting "self.hosts = self.__iter__"), presumably on the assumption that such networks represent a point-to-point link between two hosts, so the concepts of "network address" and "broadcast address" don't really apply.

    That special case seems pragmatically useful, so I think the right fix would be to:

    • document the special case that for /31 networks, hosts() includes the network and broadcast addresses (on the assumption the "network" is actually a point-to-point link between two hosts)
    • refactor IPv4Network.__init__ to first map the supplied input to a "candidate_address" and "candidate_netmask" and then use *common* validation logic to determine the actual network address and netmask (this will also address the "# fixme" comment for the int/bytes case)
    • check whether or not IPv6Network is affected by the same behavioural discrepancy

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Aug 5, 2016

    Ack, special cases! I can look into making a patch, but I'm not really acquainted with ipaddress or the relevant protocols, so it might not be optimal. I'll give it a shot next week if nobody else does.

    @zhangyangyu
    Copy link
    Member

    bpo-27683.patch tries to fix this. It alters the doc and refactors the constructor, so there is no difference between different arguments

    As for IPv6Network, it has the same problem.

    @zhangyangyu
    Copy link
    Member

    ping :)

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Sep 27, 2016
    @zhangyangyu
    Copy link
    Member

    Ping. ;-)

    @era
    Copy link
    Mannequin

    era mannequin commented Nov 1, 2016

    bpo-28577 requests a similar special case for /32

    @zhangyangyu
    Copy link
    Member

    New changeset 10b134a by Xiang Zhang in branch 'master':
    bpo-27683: Fix a regression for host() of ipaddress network objects (GH-6016)
    10b134a

    @miss-islington
    Copy link
    Contributor

    New changeset 3326c92 by Miss Islington (bot) in branch '3.7':
    bpo-27683: Fix a regression for host() of ipaddress network objects (GH-6016)
    3326c92

    @miss-islington
    Copy link
    Contributor

    New changeset ae2feb3 by Miss Islington (bot) in branch '3.6':
    bpo-27683: Fix a regression for host() of ipaddress network objects (GH-6016)
    ae2feb3

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants