commit 75f311db973412f3754eadacdd08104173f768a1 Author: Damian Johnson atagar@torproject.org Date: Tue Dec 29 10:06:16 2015 -0800

Unexpected exceptions could cause launch_tor() to leave a lingering process

Good point from germn that KeyboardInterrupts and other unxpected exception types could cause launch_tor() to leave an orphaned tor process behind...

https://trac.torproject.org/projects/tor/ticket/17946

Ensuring that when we raise an exception it gets terminated. --- docs/change_log.rst | 1 + stem/control.py | 2 +- stem/process.py | 49 +++++++++++++++++++++++++++++++------------------ test/integ/process.py | 22 ++++++++++++++++++++++ 4 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/docs/change_log.rst b/docs/change_log.rst index 87ea053..fb0a71c 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -50,6 +50,7 @@ The following are only available within Stem's `git repository * Added `support for NETWORK_LIVENESS events <api/response.html#stem.response.events.NetworkLivenessEvent>`_ (:spec:`44aac63`) * Added :func:`~stem.control.Controller.is_user_traffic_allowed` to the :class:`~stem.control.Controller` * Added the replica attribute to the :class:`~stem.response.events.HSDescEvent` (:spec:`4989e73`) + * :func:`~stem.process.launch_tor` could leave a lingering process during an unexpected exception (:trac:`17946`) * IPv6 addresses could trigger errors in :func:`~stem.control.Controller.get_listeners`, :class:`~stem.response.events.ORConnEvent`, and quite a few other things (:trac:`16174`) * Don't obscure stacktraces, most notably :class:`~stem.control.Controller` getter methods with default values * Classes with custom equality checks didn't provide a corresponding inequality method diff --git a/stem/control.py b/stem/control.py index 3b1edfa..e4509a9 100644 --- a/stem/control.py +++ b/stem/control.py @@ -1713,7 +1713,7 @@ class Controller(BaseController): raise stem.DescriptorUnavailable('Descriptor information is unavailable, tor might still be downloading it')

return stem.descriptor.server_descriptor.RelayDescriptor(desc_content) - except Exception as exc: + except: if not self._is_server_descriptors_available(): raise ValueError(SERVER_DESCRIPTORS_UNSUPPORTED)

diff --git a/stem/process.py b/stem/process.py index b8b1f92..f2d63f0 100644 --- a/stem/process.py +++ b/stem/process.py @@ -22,6 +22,7 @@ import os import re import signal import subprocess +import sys import tempfile

import stem.prereq @@ -105,27 +106,26 @@ def launch_tor(tor_cmd = 'tor', args = None, torrc_path = None, completion_perce if take_ownership: runtime_args += ['__OwningControllerProcess', str(os.getpid())]

- tor_process = subprocess.Popen(runtime_args, stdout = subprocess.PIPE, stdin = subprocess.PIPE, stderr = subprocess.PIPE) + tor_process = None

- if stdin: - tor_process.stdin.write(stem.util.str_tools._to_bytes(stdin)) - tor_process.stdin.close() + try: + tor_process = subprocess.Popen(runtime_args, stdout = subprocess.PIPE, stdin = subprocess.PIPE, stderr = subprocess.PIPE)

- if timeout: - def timeout_handler(signum, frame): - # terminates the uninitialized tor process and raise on timeout + if stdin: + tor_process.stdin.write(stem.util.str_tools._to_bytes(stdin)) + tor_process.stdin.close()

- tor_process.kill() - raise OSError('reached a %i second timeout without success' % timeout) + if timeout: + def timeout_handler(signum, frame): + raise OSError('reached a %i second timeout without success' % timeout)

- signal.signal(signal.SIGALRM, timeout_handler) - signal.alarm(timeout) + signal.signal(signal.SIGALRM, timeout_handler) + signal.alarm(timeout)

- bootstrap_line = re.compile('Bootstrapped ([0-9]+)%: ') - problem_line = re.compile('[(warn|err)] (.*)$') - last_problem = 'Timed out' + bootstrap_line = re.compile('Bootstrapped ([0-9]+)%: ') + problem_line = re.compile('[(warn|err)] (.*)$') + last_problem = 'Timed out'

- try: while True: # Tor's stdout will be read as ASCII bytes. This is fine for python 2, but # in python 3 that means it'll mismatch with other operations (for instance @@ -139,7 +139,6 @@ def launch_tor(tor_cmd = 'tor', args = None, torrc_path = None, completion_perce # this will provide empty results if the process is terminated

if not init_line: - tor_process.kill() # ... but best make sure raise OSError('Process terminated: %s' % last_problem)

# provide the caller with the initialization message if they want it @@ -162,12 +161,26 @@ def launch_tor(tor_cmd = 'tor', args = None, torrc_path = None, completion_perce msg = msg.split(': ')[-1].strip()

last_problem = msg + except: + if tor_process: + tor_process.kill() # don't leave a lingering process + tor_process.wait() + + exc = sys.exc_info()[1] + + if type(exc) == OSError: + raise # something we're raising ourselves + else: + raise OSError('Unexpected exception while starting tor (%s): %s' % (type(exc).__name__, exc)) finally: if timeout: signal.alarm(0) # stop alarm

- tor_process.stdout.close() - tor_process.stderr.close() + if tor_process and tor_process.stdout: + tor_process.stdout.close() + + if tor_process and tor_process.stderr: + tor_process.stderr.close()

if temp_file: try: diff --git a/test/integ/process.py b/test/integ/process.py index eec1e1e..3b44ef5 100644 --- a/test/integ/process.py +++ b/test/integ/process.py @@ -196,6 +196,28 @@ class TestProcess(unittest.TestCase): self.assertTrue('UseBridges' in output) self.assertTrue('SocksPort' in output)

+ @patch('re.compile', Mock(side_effect = KeyboardInterrupt('nope'))) + def test_no_orphaned_process(self): + """ + Check that when an exception arises in the middle of spawning tor that we + don't leave a lingering process. + """ + + # We don't need to actually run tor for this test. Rather, any process will + # do the trick. Picking sleep so this'll clean itself up if our test fails. + + mock_tor_process = subprocess.Popen(['sleep', '60']) + + with patch('subprocess.Popen', Mock(return_value = mock_tor_process)): + try: + stem.process.launch_tor() + self.fail("tor shoudn't have started") + except OSError as exc: + if os.path.exists('/proc/%s' % mock_tor_process.pid): + self.fail("launch_tor() left a lingering tor process") + + self.assertEqual('Unexpected exception while starting tor (KeyboardInterrupt): nope', str(exc)) + def test_torrc_arguments(self): """ Pass configuration options on the commandline.