Skip to content

Commit

Permalink
stdlib.run: Fix problems when an executable is missing
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pirat89 committed Aug 3, 2023
1 parent 154e1c5 commit ce98b43
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 6 deletions.
14 changes: 12 additions & 2 deletions leapp/libraries/stdlib/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -167,14 +167,18 @@ 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.'
api.current_logger().error(message)
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,
Expand All @@ -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:
Expand Down
27 changes: 26 additions & 1 deletion leapp/libraries/stdlib/call.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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')
Expand All @@ -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
Expand Down Expand Up @@ -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)
2 changes: 1 addition & 1 deletion leapp/models/fields/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def __init__(self, default=None, help=None): # noqa; pylint: disable=redefined-
self._nullable = False
self._default = default

if type(self) == Field:
if type(self) is Field:
raise ModelMisuseError("Do not use this type directly.")

def _validate_model_value(self, value, name):
Expand Down
30 changes: 30 additions & 0 deletions tests/scripts/test_stdlib_call_audit.py
Original file line number Diff line number Diff line change
@@ -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():
"""
Expand Down Expand Up @@ -35,3 +46,22 @@ 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'])
# this file is not executable, so even if it exists the OSError should
# be still raised
run(['panagrams'], env={'PATH': '../data/call_data/'})
11 changes: 9 additions & 2 deletions tests/scripts/test_utils_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,15 @@

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
from leapp.utils.repository import (
find_repository_basedir,
get_repository_metadata,
get_repository_name,
make_class_name,
make_name,
requires_repository,
to_snake_case
)


def setup_module():
Expand Down

0 comments on commit ce98b43

Please sign in to comment.