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

Fix for IPRoute objects that can still be used after close in Python 2 #443

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

etene
Copy link
Contributor

@etene etene commented Dec 11, 2017

Hello,

I noticed IPRoute contexts could still be used after being closed in Python 2. I assume I'm not the first one to stumble upon this, but it has caused inconsistent behavior between Python versions in our codebase so I figured it warranted a fix.

The gist of the issue is that NetlinkSocket stores references to the underlying socket methods, which can change (and do). So instead of storing references I used __getattr__ to always return the "current" methods.

The reference-based approach caused IPRoute sockets to be in an inconsistent state when closed:

  • the socket methods are replaced by dummies at closing time
  • the stored reference still points to the old method
  • the underlying os-level socket is not necessarily closed
    Thus data still could be sent over the socket even if it was considered closed.

Only Python 2 is affected, because in Python 3 socket methods point to the "real thing" and are not replaced by dummies when closing.

I guess a __getattr__ call could have a performance overhead compared to the old method, so tell me if this is a problem and I'll think of another way.

Cheers,
Étienne

…hey can change)

Use a dynamic __getattr__ based approach instead, to always use the
current socket method.

The reference-based method caused IPRoute sockets to be in an
inconsistent state when closed:
- the socket methods are replaced by dummies at closing time
- the stored reference still points to the old method
- the underlying os-level socket is not necessarily closed
Thus data still could be sent over the socket even if it was considered
closed.

Only Python 2 is affected, because in Python 3 socket methods point to
the "real thing" and are not replaced by dummies when closing.
@svinota
Copy link
Owner

svinota commented Jan 15, 2018

Thanks! Testing

openstack-gerrit pushed a commit to openstack/os-vif that referenced this pull request Jan 17, 2018
- This change adds an iptools driver
  based on the process exec.
- This driver is a workaround for a file handle
  leak in the current release of pyroute2.
  svinota/pyroute2#443
- This driver is considered deprecated and will
  be removed when a new release of pyroute2 is
  available.

Change-Id: Idbb3c6b09ff8a883863925cd0db9b552b726a54d
openstack-gerrit pushed a commit to openstack/requirements that referenced this pull request Jan 19, 2018
- This change blacklists os-vif 1.8.0 as it breaks
  compatiblity with kuryr-kubernetes
  (Ie1b65fb29b6abcf77a531404e67c7e60b2183c19)
  and introduces a file handel leak  due to an
  upstream bug in pyroute2
  (svinota/pyroute2#443).

Depends-On: Id05ed5dced8ca26800585b98d65c0a12570773e2
Change-Id: I955022a9dc7df8ee0a73155f8ab514bd51ce4004
meta:version: 1.9.0
meta:diff-start: -
meta:series: queens
meta:release-type: release
meta:pypi: yes
meta:first: no
meta:release:Author: Sean Mooney <sean.k.mooney@intel.com>
meta:release:Commit: Sean Mooney <sean.k.mooney@intel.com>
meta:release:Change-Id: I1d0b52bb8893e6b892af579c0a5d3216859f1e5b
meta:release:Code-Review+1: Sylvain Bauza <sbauza@redhat.com>
meta:release:Code-Review+2: Sean McGinnis <sean.mcginnis@gmail.com>
meta:release:Workflow+1: Sean McGinnis <sean.mcginnis@gmail.com>
@svinota svinota merged commit 573ba9f into svinota:master Jan 31, 2018
@svinota
Copy link
Owner

svinota commented Jan 31, 2018

Thanks!

tanaypf9 pushed a commit to tanaypf9/pf9-requirements that referenced this pull request May 20, 2024
Patch Set 4:

at present neutron does not consume os-vif.
nova has defensive coding around populating new vif fields introduced in 1.8.0/1.9.0 such as the new datapath_type introduced in VIFPortProfileOpenVSwitch

while the pyroute2 implementation is technically functional in 1.8.0 we have disabled until such a time as the upstream bug is fixed.

neutron uses pyroute2 also so ye may be impacted by svinota/pyroute2#443 under python 2

Patch-set: 4
tanaypf9 pushed a commit to tanaypf9/pf9-requirements that referenced this pull request May 20, 2024
Patch Set 3:

ya, I'm thinking pyroute2 is the cause

see svinota/pyroute2#443

Patch-set: 3
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.

None yet

2 participants