From d27a57cb49893d6edb192492636d267fb74b9cd9 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Thu, 11 Sep 2014 13:13:42 -0700 Subject: [PATCH] Avoid Popen() of executables that don't exist The stdlib 'subprocess' module in python-2.7.4 through 2.7.7 suffers from http://bugs.python.org/issue18851 which causes unrelated file descriptors to be closed when `subprocess.call()` fails the `exec()`, such as when the executable being invoked does not actually exist. There appears to be some randomness involved. This was fixed in python-2.7.8. Tahoe's iputil.py uses subprocess.call on many different "ifconfig"-type executables, most of which don't exist on any given platform (added in git commit 8e31d66cd0b). This results in a lot of file-descriptor closing, which (at least during unit tests) tends to clobber important things like Tub TCP sockets. This seems to be the root cause behind ticket:2121, in which normal code tries to close already-closed sockets, crashing the unit tests. Since different platforms have different ifconfigs, some platforms will experience more failed execs than others, so this bug could easily behave differently on linux vs freebsd, as well as working normally on python-2.7.8 or 2.7.4. This patch inserts a guard to make sure that os.path.isfile() is true before allowing Popen.call() to try executing the target. This ought to be enough to avoid the bug. It changes both iputil.py and allmydata.__init__ (which uses Popen for calling "lsb_release"), which are all the places where 'subprocess' is used outside of unit tests. Other potential fixes: use the 'subprocess32' module from PyPI (which is a bug-free backport of the Python3 stdlib subprocess module, but would introduce a new dependency), or require python >= 2.7.8 (but this would rule out development/deployment on the current OS-X 10.9 release, which ships with 2.7.5, as well as other distributions like Ubuntu 14.04 LTS). I believe this closes ticket:2121, and given the apparent relationship between 2121 and 2023, I think it also closes ticket:2023 (although since 2023 doesn't have copies of the failing log files, it's hard to tell). I'm hoping that this will tide us over until 1.11 is released, at which point we can execute on the plan to remove iputil.py entirely by changing the way that nodes learn their externally-facing IP address. --- src/allmydata/__init__.py | 37 ++++++++++++++++--------------- src/allmydata/test/test_iputil.py | 1 + src/allmydata/util/iputil.py | 2 ++ 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/allmydata/__init__.py b/src/allmydata/__init__.py index 18d15f64..95019aaf 100644 --- a/src/allmydata/__init__.py +++ b/src/allmydata/__init__.py @@ -106,24 +106,25 @@ def get_linux_distro(): if _distname and _version: return (_distname, _version) - try: - p = subprocess.Popen(["lsb_release", "--all"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - rc = p.wait() - if rc == 0: - for line in p.stdout.readlines(): - m = _distributor_id_cmdline_re.search(line) - if m: - _distname = m.group(1).strip() - if _distname and _version: - return (_distname, _version) - - m = _release_cmdline_re.search(p.stdout.read()) - if m: - _version = m.group(1).strip() - if _distname and _version: - return (_distname, _version) - except EnvironmentError: - pass + if os.path.isfile("/usr/bin/lsb_release") or os.path.isfile("/bin/lsb_release"): + try: + p = subprocess.Popen(["lsb_release", "--all"], stdout=subprocess.PIPE, stderr=subprocess.PIPE) + rc = p.wait() + if rc == 0: + for line in p.stdout.readlines(): + m = _distributor_id_cmdline_re.search(line) + if m: + _distname = m.group(1).strip() + if _distname and _version: + return (_distname, _version) + + m = _release_cmdline_re.search(p.stdout.read()) + if m: + _version = m.group(1).strip() + if _distname and _version: + return (_distname, _version) + except EnvironmentError: + pass if os.path.exists("/etc/arch-release"): return ("Arch_Linux", "") diff --git a/src/allmydata/test/test_iputil.py b/src/allmydata/test/test_iputil.py index 6a5b08f3..a8d4569d 100644 --- a/src/allmydata/test/test_iputil.py +++ b/src/allmydata/test/test_iputil.py @@ -130,6 +130,7 @@ class ListAddresses(testutil.SignalMixin, unittest.TestCase): e.errno = errno.ENOENT raise e self.patch(subprocess, 'Popen', call_Popen) + self.patch(os.path, 'isfile', lambda x: True) def call_get_local_ip_for(target): if target in ("localhost", "127.0.0.1"): diff --git a/src/allmydata/util/iputil.py b/src/allmydata/util/iputil.py index 2f7db051..4ea6aa6f 100644 --- a/src/allmydata/util/iputil.py +++ b/src/allmydata/util/iputil.py @@ -190,6 +190,8 @@ def _synchronously_find_addresses_via_config(): return [] def _query(path, args, regex): + if not os.path.isfile(path): + return [] env = {'LANG': 'en_US.UTF-8'} TRIES = 5 for trial in xrange(TRIES): -- 2.45.2