Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[crmsh-4.6] Fix: sh: pass env to child process explicitly (bsc#1205925) #1356

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crmsh/report/sh.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def subprocess_run_without_input(self, cmd: str, **kwargs):
return subprocess.run(
args,
input=cmd.encode('utf-8'),
env=os.environ, # bsc#1205925
**kwargs,
)

Expand Down
10 changes: 8 additions & 2 deletions crmsh/scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,9 @@ def _check_control_persist():
print((".EXT", cmd))
cmd = subprocess.Popen(cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
stderr=subprocess.PIPE,
env=os.environ, # bsc#1205925
)
(out, err) = cmd.communicate()
return "Bad configuration option" not in err

Expand Down Expand Up @@ -1138,7 +1140,11 @@ def _cleanup_local(workdir):
if workdir and os.path.isdir(workdir):
cleanscript = os.path.join(workdir, 'crm_clean.py')
if os.path.isfile(cleanscript):
if subprocess.call([cleanscript, workdir], shell=False) != 0:
if subprocess.call(
[cleanscript, workdir],
shell=False,
env=os.environ, # bsc#1205925
) != 0:
shutil.rmtree(workdir)
else:
shutil.rmtree(workdir)
Expand Down
12 changes: 9 additions & 3 deletions crmsh/sh.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,12 @@ def su_subprocess_run(
)
if not self.additional_environ:
logger.debug('su_subprocess_run: %s, %s', args, kwargs)
return subprocess.run(args, **kwargs)
env = os.environ # bsc#1205925
else:
logger.debug('su_subprocess_run: %s, env=%s, %s', args, self.additional_environ, kwargs)
env = dict(os.environ)
env.update(self.additional_environ)
return subprocess.run(args, env=env, **kwargs)
return subprocess.run(args, env=env, **kwargs)

def get_rc_stdout_stderr_raw(self, user: typing.Optional[str], cmd: str, input: typing.Optional[bytes] = None):
result = self.su_subprocess_run(
Expand Down Expand Up @@ -268,6 +268,7 @@ def subprocess_run_without_input(self, host: typing.Optional[str], user: str, cm
return subprocess.run(
args,
input=cmd.encode('utf-8'),
env=os.environ, # bsc#1205925
**kwargs,
)
else:
Expand Down Expand Up @@ -317,6 +318,7 @@ def subprocess_run_without_input(self, host: typing.Optional[str], user: typing.
return subprocess.run(
['/bin/sh'],
input=cmd.encode('utf-8'),
env=os.environ, # bsc#1205925
**kwargs,
)
else:
Expand Down Expand Up @@ -436,6 +438,7 @@ def get_stdout(cls, cmd, input_s=None, stderr_on=True, shell=True, raw=False):
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL if stderr_on else subprocess.PIPE,
env=os.environ, # bsc#1205925
)
stdout_data, _ = proc.communicate(input_s)
if raw:
Expand All @@ -457,7 +460,9 @@ def get_stdout_stderr(cls, cmd, input_s=None, shell=True, raw=False, no_reg=Fals
shell=shell,
stdin=input_s and subprocess.PIPE or None,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
stderr=subprocess.PIPE,
env=os.environ, # bsc#1205925
)
# will raise subprocess.TimeoutExpired if set timeout
stdout_data, stderr_data = proc.communicate(input_s, timeout=timeout)
if raw:
Expand Down Expand Up @@ -489,3 +494,4 @@ def subprocess_run_without_input(self, host: str, user: typing.Optional[str], cm

def cluster_shell():
return ClusterShell(LocalShell(), user_of_host.instance(), raise_ssh_error=True)

11 changes: 10 additions & 1 deletion crmsh/term.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Copyright (C) 2008-2011 Dejan Muhamedagic <[email protected]>
# See COPYING for license information.

import os
import sys
import re
# from: http://code.activestate.com/recipes/475116/
Expand Down Expand Up @@ -151,6 +151,7 @@
from . import config
if not _term_stream.isatty() and 'color-always' not in config.color.style:
return
_ignore_environ()

Check warning on line 154 in crmsh/term.py

View check run for this annotation

Codecov / codecov/patch

crmsh/term.py#L154

Added line #L154 was not covered by tests
# Check the terminal type. If we fail, then assume that the
# terminal has no capabilities.
try:
Expand All @@ -177,4 +178,12 @@
return hasattr(colors, s.upper())


def _ignore_environ():
"""Ignore environment variable COLUMNS and ROWS"""
# See https://bugzilla.suse.com/show_bug.cgi?id=1205925
# and https://gitlab.com/procps-ng/procps/-/blob/c415fc86452c933716053a50ab1777a343190dcc/src/ps/global.c#L279
for name in ["COLUMNS", "ROWS"]:
if name in os.environ:
del os.environ[name]

Check warning on line 187 in crmsh/term.py

View check run for this annotation

Codecov / codecov/patch

crmsh/term.py#L185-L187

Added lines #L185 - L187 were not covered by tests

# vim:ts=4:sw=4:et:
11 changes: 7 additions & 4 deletions crmsh/ui_node.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Copyright (C) 2008-2011 Dejan Muhamedagic <[email protected]>
# Copyright (C) 2013 Kristoffer Gronlund <[email protected]>
# See COPYING for license information.

import os
import re
import copy
import subprocess
Expand Down Expand Up @@ -550,10 +550,13 @@
'usage: delete <node>'
logger.warning('`crm node delete` is deprecated and will very likely be dropped in the near future. It is auto-replaced as `crm cluster remove -c {}`.'.format(node))
if config.core.force:
rc = subprocess.call(['crm', 'cluster', 'remove', '-F', '-c', node])
args = ['crm', 'cluster', 'remove', '-F', '-c', node]
else:
rc = subprocess.call(['crm', 'cluster', 'remove', '-c', node])
return rc == 0
args = ['crm', 'cluster', 'remove', '-c', node]

Check warning on line 555 in crmsh/ui_node.py

View check run for this annotation

Codecov / codecov/patch

crmsh/ui_node.py#L555

Added line #L555 was not covered by tests
return 0 == subprocess.call(
args,
env=os.environ, # bsc#1205925
)

@command.wait
@command.completers(compl.nodes, compl.choice(['set', 'delete', 'show']), _find_attr)
Expand Down
2 changes: 2 additions & 0 deletions crmsh/user_of_host.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import os
import socket
import subprocess
import typing
Expand Down Expand Up @@ -107,6 +108,7 @@ def _guess_user_for_ssh(host: str) -> typing.Tuple[str, str]:
stdin=subprocess.DEVNULL,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
env=os.environ, # bsc#1205925
)
if rc == 0:
user = userdir.getuser()
Expand Down
36 changes: 28 additions & 8 deletions crmsh/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,14 @@


def set_cib_in_use(name):
os.putenv(_cib_shadow, name)
os.environ[_cib_shadow] = name

Check warning on line 153 in crmsh/utils.py

View check run for this annotation

Codecov / codecov/patch

crmsh/utils.py#L153

Added line #L153 was not covered by tests
global _cib_in_use
_cib_in_use = name


def clear_cib_in_use():
os.unsetenv(_cib_shadow)
if _cib_shadow in os.environ:
del os.environ[_cib_shadow]

Check warning on line 160 in crmsh/utils.py

View check run for this annotation

Codecov / codecov/patch

crmsh/utils.py#L159-L160

Added lines #L159 - L160 were not covered by tests
global _cib_in_use
_cib_in_use = ''

Expand Down Expand Up @@ -523,7 +524,12 @@
logger.debug("piping string to %s", cmd)
if options.regression_tests:
print(".EXT", cmd)
p = subprocess.Popen(cmd, shell=True, stdin=subprocess.PIPE)
p = subprocess.Popen(
cmd,
shell=True,
stdin=subprocess.PIPE,
env=os.environ, # bsc#1205925
)
try:
# communicate() expects encoded bytes
if isinstance(s, str):
Expand Down Expand Up @@ -552,7 +558,9 @@
shell=shell,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=stderr)
stderr=stderr,
env=os.environ, # bsc#1205925
)
try:
# bytes expected here
if isinstance(s, str):
Expand Down Expand Up @@ -770,7 +778,9 @@
if options.regression_tests:
print(".EXT", cmd)
subprocess.Popen(cmd, shell=True, bufsize=0,
stdin=None, stdout=None, stderr=None, close_fds=True)
stdin=None, stdout=None, stderr=None, close_fds=True,
env=os.environ, # bsc#1205925
)
logger.info("starting %s to show %s", config.core.dotty, desc)


Expand All @@ -779,13 +789,21 @@
if options.regression_tests:
print(".EXT", cmd)
logger.debug("invoke: %s", cmd)
return subprocess.call(cmd, shell=shell)
return subprocess.call(
cmd,
shell=shell,
env=os.environ, # bsc#1205925
)


def ext_cmd_nosudo(cmd, shell=True):
if options.regression_tests:
print(".EXT", cmd)
return subprocess.call(cmd, shell=shell)
return subprocess.call(

Check warning on line 802 in crmsh/utils.py

View check run for this annotation

Codecov / codecov/patch

crmsh/utils.py#L802

Added line #L802 was not covered by tests
cmd,
shell=shell,
env=os.environ, # bsc#1205925
)


def rmdir_r(d):
Expand Down Expand Up @@ -918,7 +936,9 @@
proc = subprocess.Popen(cmd,
shell=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
stderr=subprocess.PIPE,
env=os.environ, # bsc#1205925
)
(outp, err_outp) = proc.communicate()
proc.wait()
rc = proc.returncode
Expand Down
6 changes: 5 additions & 1 deletion crmsh/xmlutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ def sudocall(cmd):
cmd = add_sudo(cmd)
if options.regression_tests:
print(".EXT", cmd)
p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
p = subprocess.Popen(
cmd, shell=True,
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
env=os.environ, # bsc#1205925
)
try:
outp, errp = p.communicate()
p.wait()
Expand Down
12 changes: 9 additions & 3 deletions test/unittests/test_sh.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ def setUp(self) -> None:
self.local_shell.geteuid = mock.Mock(self.local_shell.geteuid)
self.local_shell.hostname = mock.Mock(self.local_shell.hostname)

@mock.patch('os.environ')
@mock.patch('subprocess.run')
def test_su_subprocess_run(self, mock_run: mock.MagicMock):
def test_su_subprocess_run(self, mock_run: mock.MagicMock, mock_environ: mock.MagicMock):
self.local_shell.get_effective_user_name.return_value = 'root'
self.local_shell.geteuid.return_value = 0
self.local_shell.su_subprocess_run(
Expand All @@ -24,10 +25,12 @@ def test_su_subprocess_run(self, mock_run: mock.MagicMock):
mock_run.assert_called_once_with(
['su', 'alice', '--login', '-s', '/bin/sh', '-c', 'foo'],
input=b'bar',
env=mock_environ,
)

@mock.patch('os.environ')
@mock.patch('subprocess.run')
def test_su_subprocess_run_as_root(self, mock_run: mock.MagicMock):
def test_su_subprocess_run_as_root(self, mock_run: mock.MagicMock, mock_environ: mock.MagicMock):
self.local_shell.get_effective_user_name.return_value = 'root'
self.local_shell.geteuid.return_value = 0
self.local_shell.su_subprocess_run(
Expand All @@ -37,6 +40,7 @@ def test_su_subprocess_run_as_root(self, mock_run: mock.MagicMock):
mock_run.assert_called_once_with(
['/bin/sh', '-c', 'foo'],
input=b'bar',
env=mock_environ,
)

@mock.patch('subprocess.run')
Expand Down Expand Up @@ -124,8 +128,9 @@ def test_subprocess_run_without_input_with_input_kwargs(self):
)
self.ssh_shell.local_shell.su_subprocess_run.assert_not_called()

@mock.patch('os.environ')
@mock.patch('subprocess.run')
def test_subprocess_run_without_input_local(self, mock_run):
def test_subprocess_run_without_input_local(self, mock_run, mock_environ: mock.MagicMock):
self.ssh_shell.subprocess_run_without_input(
'node1', 'bob',
'foo',
Expand All @@ -138,6 +143,7 @@ def test_subprocess_run_without_input_local(self, mock_run):
input=b'foo',
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
env=mock_environ,
)


Expand Down
Loading