From 84ab1e4c54b59ae11624cf48bdc3e06d9d9decdf Mon Sep 17 00:00:00 2001 From: Petr Stodulka Date: Mon, 17 Jul 2023 17:18:52 +0200 Subject: [PATCH] stdlib.run: Fix problems when an executable is missing Currently, if the executable required in `run` does not exist (or is not executable), the child process's code is not replaced by `os.execvpe` function and it raises the OSError instead. However, the parent process does not get this OSError. It consumes exit code, stderr, ... of the child process. So in case the code does something like this: ``` try: result = run(['non-executable']) except OSError: pass except CalledProcessError: # do something.. ``` the child process passes and do whatever.. In case it ends with zero exit code, the obtained result is usually something totally different than expected in actors. Also there could be problems with non-idempotent code, when some actions could be done twice (once executed by the child, send time executed by the parent [current] process). We have realized that number of existing leapp actors for in-place upgrades already count with the raise of OSError when executable cannot be used. So we choosed for now to check whether executables are present and raise OSError if not, so we are sure that only one process leave the function really. Also applied another seatbelt into the child process - if the OSError is raised anyway despite our checks (e.g. SELinux prevents the execution) let's just kill the process instead giving it a possibility to continue. In such a case, always print a msg to stderr of the child process and exit with ecode 1. Note: To check an executable we use `distutils.spawn.find_executable` which is deprecated in Python 3 and will be dropped in Python 3.12. However it exists now for Python 2 & 3, so we use this one for now and will replace it in future by `shutils.which` when the time comes. Additional changes: * Make pylint happy (set noqa for E721 for the check the Fields is not created directly). The "isinstance" function has a different behaviour in this case than we want. * Fix imports to make pylint happy * Set `result` to empty dict instead of None: in case a ValueError or TypeError is raised inside the _call function, it's expected to copy the `result` content inside the final block, however in case the results is None, it fails and raise additional exception that covers the original one. * Update unit tests to cover issues with missing executable. --- leapp/libraries/stdlib/__init__.py | 14 +++++++++-- leapp/libraries/stdlib/call.py | 27 ++++++++++++++++++++- tests/scripts/test_stdlib_call_audit.py | 32 +++++++++++++++++++++++++ tests/scripts/test_utils_project.py | 10 ++++---- 4 files changed, 75 insertions(+), 8 deletions(-) diff --git a/leapp/libraries/stdlib/__init__.py b/leapp/libraries/stdlib/__init__.py index 95974964a..77a2f7a29 100644 --- a/leapp/libraries/stdlib/__init__.py +++ b/leapp/libraries/stdlib/__init__.py @@ -3,11 +3,11 @@ represents a location for functions that otherwise would be defined multiple times across leapp actors and at the same time, they are really useful for other actors. """ +import base64 import logging import os import sys import uuid -import base64 from leapp.exceptions import LeappError from leapp.utils.audit import create_audit_entry @@ -167,6 +167,10 @@ def run(args, split=False, callback_raw=_console_logging_handler, callback_lineb :type stdin: int, str :return: {'stdout' : stdout, 'stderr': stderr, 'signal': signal, 'exit_code': exit_code, 'pid': pid} :rtype: dict + :raises: OSError if an executable is missing or has wrong permissions + :raises: CalledProcessError if the cmd has non-zero exit code and `checked` is False + :raises: TypeError if any input parameters have an invalid type + :raises: valueError if any of input parameters have an invalid value """ if not args: message = 'Command to call is missing.' @@ -174,7 +178,7 @@ def run(args, split=False, callback_raw=_console_logging_handler, callback_lineb raise ValueError(message) api.current_logger().debug('External command has started: {0}'.format(str(args))) _id = str(uuid.uuid4()) - result = None + result = {} try: create_audit_entry('process-start', {'id': _id, 'parameters': args, 'env': env}) result = _call(args, callback_raw=callback_raw, callback_linebuffered=callback_linebuffered, @@ -191,6 +195,12 @@ def run(args, split=False, callback_raw=_console_logging_handler, callback_lineb result.update({ 'stdout': result['stdout'].splitlines() }) + except OSError: + # NOTE: currently we expect the result to be always set + # let's copy bash a little bit and set ecode 127 + result = {'exit_code': '127', 'stdout': '', 'signal': 0, 'pid': 0} + result['stderr'] = 'File not found or permission denied: {}'.format(args[0]) + raise finally: audit_result = result if not encoding: diff --git a/leapp/libraries/stdlib/call.py b/leapp/libraries/stdlib/call.py index 719bbd615..363fd279e 100644 --- a/leapp/libraries/stdlib/call.py +++ b/leapp/libraries/stdlib/call.py @@ -1,7 +1,10 @@ from __future__ import print_function +from distutils.spawn import find_executable import codecs +import errno import os +import sys from leapp.compat import string_types from leapp.libraries.stdlib.eventloop import POLL_HUP, POLL_IN, POLL_OUT, POLL_PRI, EventLoop @@ -107,6 +110,10 @@ def _call(command, callback_raw=lambda fd, value: None, callback_linebuffered=la :type env: dict :return: {'stdout' : stdout, 'stderr': stderr, 'signal': signal, 'exit_code': exit_code, 'pid': pid} :rtype: dict + :raises: OSError if an executable is missing or has wrong permissions + :raises: CalledProcessError if the cmd has non-zero exit code and `checked` is False + :raises: TypeError if any input parameters have an invalid type + :raises: valueError if any of input parameters have an invalid value """ if not isinstance(command, (list, tuple)): raise TypeError('command parameter has to be a list or tuple') @@ -126,6 +133,17 @@ def _call(command, callback_raw=lambda fd, value: None, callback_linebuffered=la if not isinstance(env, dict): raise TypeError('env parameter has to be a dictionary') environ.update(env) + + _path = (env or {}).get('PATH', None) + # NOTE(pstodulk): the find_executable function is from the distutils + # module which is deprecated and it is going to be removed in Python 3.12. + # In future, we should use the shutil.which function, however that one is + # not available for Python2. We are going to address the problem in future + # (e.g. when we drop support for Python 2). + # https://peps.python.org/pep-0632/ + if not find_executable(command[0], _path): + raise OSError(errno.ENOENT, os.strerror(errno.ENOENT), command[0]) + # Create a separate pipe for stdout/stderr # # The parent process is going to use the read-end of the pipes for reading child's @@ -214,4 +232,11 @@ def _call(command, callback_raw=lambda fd, value: None, callback_linebuffered=la os.close(stderr) os.dup2(wstdout, STDOUT) os.dup2(wstderr, STDERR) - os.execvpe(command[0], command, env=environ) + try: + os.execvpe(command[0], command, env=environ) + except OSError as e: + # This is a seatbelt in case the execvpe cannot be performed + # (e.g. permission denied) and we didn't catch this prior the fork. + # See the PR for more details: https://github.com/oamg/leapp/pull/836 + sys.stderr.write('Error: Cannot execute {}: {}\n'.format(command[0], str(e))) + os._exit(1) diff --git a/tests/scripts/test_stdlib_call_audit.py b/tests/scripts/test_stdlib_call_audit.py index 7f61e8307..521358e95 100644 --- a/tests/scripts/test_stdlib_call_audit.py +++ b/tests/scripts/test_stdlib_call_audit.py @@ -1,8 +1,19 @@ +import os import pytest import six from leapp.libraries.stdlib import run +CUR_DIR = os.path.dirname(os.path.abspath(__file__)) + + +@pytest.fixture +def adjust_cwd(): + previous_cwd = os.getcwd() + os.chdir(os.path.join(CUR_DIR, "../")) + yield + os.chdir(previous_cwd) + def test_invalid_command(): """ @@ -35,3 +46,24 @@ def test_no_encoding(): cmd = ['echo', '-n', '-e', '\\xeb'] result = run(cmd, encoding=None) assert isinstance(result['stdout'], six.binary_type) + + +def test_missing_executable(monkeypatch, adjust_cwd): # no-qa: W0613; pylint: disable=unused-argument + """ + In case the executable is missing. + + :return: Pass/Fail + """ + def mocked_fork(): + # raise the generic exception as we want to prevent fork in this case + # and want to bypass possible catch + raise Exception() # no-qa: W0719; pylint: disable=broad-exception-raised + + monkeypatch.setattr(os, 'fork', mocked_fork) + with pytest.raises(OSError): + run(['my-non-existing-exec-2023-asdf']) + + with pytest.raises(OSError): + # this file is not executable, so even if it exists the OSError should + # be still raised + run(['panagrams'], env={'PATH': '../data/call_data/'}) diff --git a/tests/scripts/test_utils_project.py b/tests/scripts/test_utils_project.py index c58a8a6b4..33b09c595 100644 --- a/tests/scripts/test_utils_project.py +++ b/tests/scripts/test_utils_project.py @@ -5,13 +5,13 @@ from helpers import TESTING_REPOSITORY_NAME from leapp.exceptions import CommandError from leapp.utils.repository import ( - requires_repository, - to_snake_case, - make_class_name, - make_name, find_repository_basedir, - get_repository_name, get_repository_metadata, + get_repository_name, + make_class_name, + make_name, + requires_repository, + to_snake_case, )