File 0001-netns-fix-NetNS-resource-leakage-504.patch of Package python-pyroute2

Overview Repositories Revisions Requests Users Attributes Meta

File 0001-netns-fix-NetNS-resource-leakage-504.patch of Package python-pyroute2

From 652a3136d993d438142cad3602101c5dd9654ae2 Mon Sep 17 00:00:00 2001
From: Zhengsheng Zhou <edwardbadboy@users.noreply.github.com>
Date: Mon, 21 May 2018 15:47:23 +0800
Subject: [PATCH] netns: fix NetNS resource leakage (#504)

This patch catches the exception during the NetNS object instantiation,
and explicitly releases the related FDs.

Once the child process has been forked, the parent process and the
child process should close the unused ends of the socket pairs, so
after the parent process closes "cmdch", the "select.select" invocation
in child process returns. The child process can exit automatically
when the parment process exits or is killed.

To prevent memory leakage, this patch also removes the registered
atexit cleanup function when NetNS is closed successfully.

This patch also removes the workaround for
https://bugs.python.org/issue27151 . This is because though the NetNS
object was closed, but previously it cannot be garbage collected since
there was still a reference from "atexit registry --> NetNS.close -->
NetNS". So the NetNS's self.server(a Process object) along with its
sentinel FD cannot be garbage collected. After this patch, everything
can be garbage collected by GC. In Python 3.5, every Process object is
registered to a global finalizing list using weakref. When gc about to
delete the object, it invokes the finalizer callback and the FD is
closed actually.

Overall, there are 3 cases:

1. Instantiate NetNS with non-existing netns and flags=0.
    try:
        ns = NetNS('xyz', flags=0)
    except Exception:
        pass
Without this patch, socket pairs and memory are leaked.

2. Instantiate NetNS with existing netns, then close NetNS.
    with NetNS('xyz') as ns:
        pass
Without this patch, memory is leaked. Each time NetNS instatiates,
"atexit._exithandlers" list gets longer.

3. Instantiate NetNS, when using it, kill the parent process.
    ns = NetNS('xyz')
    time.sleep(100)
    # then kill the Python interpreter using SIGKILL
Without the patch, the child process will not exit.

Bug-Url: https://github.com/svinota/pyroute2/issues/501
Signed-off-by: Zhengsheng Zhou <zhengshengz@vmware.com>
(cherry picked from commit e448c5bbada52da9ccdfb2a3951c71f5e0269579)

Conflicts:
	tests/general/test_stress.py
---
 pyroute2/netns/nslink.py     | 59 ++++++++++++++++++++++--------------
 pyroute2/remote/__init__.py  | 10 ++++++

diff --git a/pyroute2/netns/nslink.py b/pyroute2/netns/nslink.py
index 3f8417f4..661bbafb 100644
--- a/pyroute2/netns/nslink.py
+++ b/pyroute2/netns/nslink.py
@@ -66,10 +66,8 @@ run `remove()`.
 
 import os
 import errno
-import fcntl
 import atexit
 import signal
-import sys
 import logging
 from pyroute2 import config
 from pyroute2.netlink.rtnl.iprsocket import MarshalRtnl
@@ -82,7 +80,8 @@ from pyroute2.remote import RemoteSocket
 log = logging.getLogger(__name__)
 
 
-def NetNServer(netns, cmdch, brdch, flags=os.O_CREAT):
+def NetNServer(netns, parent_cmdch, cmdch, parent_brdch, brdch,
+               flags=os.O_CREAT):
     '''
     The netns server supposed to be started automatically by NetNS.
     It has two communication channels: one simplex to forward incoming
@@ -137,6 +136,8 @@ def NetNServer(netns, cmdch, brdch, flags=os.O_CREAT):
                     'error': OSError(errno.ECOMM, str(e), netns)})
         return 255
 
+    parent_cmdch.close()
+    parent_brdch.close()
     Server(cmdch, brdch)
     os.close(nsfd)
 
@@ -181,43 +182,57 @@ class NetNS(IPRouteMixin, RemoteSocket):
     def __init__(self, netns, flags=os.O_CREAT):
         self.netns = netns
         self.flags = flags
-        self.cmdch, self._cmdch = config.MpPipe()
-        self.brdch, self._brdch = config.MpPipe()
-        atexit.register(self.close)
+        self.cmdch, _cmdch = config.MpPipe()
+        self.brdch, _brdch = config.MpPipe()
         self.server = config.MpProcess(target=NetNServer,
                                        args=(self.netns,
-                                             self._cmdch,
-                                             self._brdch,
+                                             self.cmdch,
+                                             _cmdch,
+                                             self.brdch,
+                                             _brdch,
                                              self.flags))
         self.server.start()
-        super(NetNS, self).__init__()
+        _cmdch.close()
+        _brdch.close()
+        try:
+            super(NetNS, self).__init__()
+        except Exception:
+            self.close()
+            raise
+        atexit.register(self.close)
         self.marshal = MarshalRtnl()
 
     def clone(self):
         return type(self)(self.netns, self.flags)
 
+    def _cleanup_atexit(self):
+        if hasattr(atexit, 'unregister'):
+            atexit.unregister(self.close)
+        else:
+            try:
+                atexit._exithandlers.remove((self.close, (), {}))
+            except ValueError:
+                pass
+
     def close(self):
+        self._cleanup_atexit()
         try:
             super(NetNS, self).close()
         except:
             # something went wrong, force server shutdown
-            self.cmdch.send({'stage': 'shutdown'})
+            try:
+                self.cmdch.send({'stage': 'shutdown'})
+            except Exception:
+                pass
             log.error('forced shutdown procedure, clean up netns manually')
         # force cleanup command channels
-        self.cmdch.close()
-        self.brdch.close()
-        self._cmdch.close()
-        self._brdch.close()
+        for close in (self.cmdch.close, self.brdch.close):
+            try:
+                close()
+            except Exception:
+                pass  # Maybe already closed in remote.Client.close
         # join the server
         self.server.join()
-        # Workaround for http://bugs.python.org/issue27151
-        if sys.version_info > (3, 2):
-            try:
-                fcntl.fcntl(self.server.sentinel, fcntl.F_GETFD)
-            except:
-                pass
-            else:
-                os.close(self.server.sentinel)
 
     def post_init(self):
         pass
diff --git a/pyroute2/remote/__init__.py b/pyroute2/remote/__init__.py
index 7f1ce1fa..9dec2971 100644
--- a/pyroute2/remote/__init__.py
+++ b/pyroute2/remote/__init__.py
@@ -281,10 +281,20 @@ class Client(object):
             raise msg['error']
         return msg['data']
 
+    def _cleanup_atexit(self):
+        if hasattr(atexit, 'unregister'):
+            atexit.unregister(self.close)
+        else:
+            try:
+                atexit._exithandlers.remove((self.close, (), {}))
+            except ValueError:
+                pass
+
     def close(self):
         with self.lock:
             if not self.closed:
                 self.closed = True
+                self._cleanup_atexit()
                 self.cmdch.send({'stage': 'shutdown'})
                 if hasattr(self.cmdch, 'close'):
                     self.cmdch.close()
-- 
2.17.1

Places

File 0001-netns-fix-NetNS-resource-leakage-504.patch of Package python-pyroute2

Places