From 8b6308b6c18095f0f48f85ae262719aedb9105da Mon Sep 17 00:00:00 2001
From: Nicholas Yang <nicholas.yang@suse.com>
Date: Wed, 13 Mar 2024 17:35:48 +0800
Subject: [PATCH 1/5] Fix: sh: pass env to child process explicitly
(bsc#1205925)
A bug in python stardard library, python/cpython#100516, make
environment variables, `COLUMNS` and `ROWS` to be added at fork. These
environment variables may make some of the commands called by crmsh to
truncate its output even if those commands are not writing to the
terminal.
Passing argument `env` explicitly is a workaround for the python bug.
---
crmsh/report/sh.py | 1 +
crmsh/sh.py | 12 +++++++++---
crmsh/user_of_host.py | 2 ++
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/crmsh/report/sh.py b/crmsh/report/sh.py
index 46f981a863..c3627fcfe1 100644
--- a/crmsh/report/sh.py
+++ b/crmsh/report/sh.py
@@ -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,
)
diff --git a/crmsh/sh.py b/crmsh/sh.py
index 5b9c052621..16470e3a5d 100644
--- a/crmsh/sh.py
+++ b/crmsh/sh.py
@@ -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(
@@ -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:
@@ -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:
@@ -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:
@@ -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:
@@ -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)
+
diff --git a/crmsh/user_of_host.py b/crmsh/user_of_host.py
index 4551dce978..9c990daec3 100644
--- a/crmsh/user_of_host.py
+++ b/crmsh/user_of_host.py
@@ -1,4 +1,5 @@
import logging
+import os
import socket
import subprocess
import typing
@@ -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()
From b901b4ef236e84f1a56fd6b05fc796935a3dd242 Mon Sep 17 00:00:00 2001
From: Nicholas Yang <nicholas.yang@suse.com>
Date: Wed, 13 Mar 2024 17:55:52 +0800
Subject: [PATCH 2/5] Dev: unittest: update unit tests for previous changes
---
test/unittests/test_sh.py | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/test/unittests/test_sh.py b/test/unittests/test_sh.py
index b3c0f0bb11..221820afd2 100644
--- a/test/unittests/test_sh.py
+++ b/test/unittests/test_sh.py
@@ -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(
@@ -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(
@@ -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')
@@ -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',
@@ -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,
)
From b604b9b9d42b315403117e2d2c6c351312151c25 Mon Sep 17 00:00:00 2001
From: Nicholas Yang <nicholas.yang@suse.com>
Date: Wed, 13 Mar 2024 17:49:05 +0800
Subject: [PATCH 3/5] Fix: term: unset env `COLUMNS` and `ROWS` (bsc#1205925)
A bug in python stardard library, python/cpython#100516, make
environment variables, `COLUMNS` and `ROWS` to be added at fork. These
environment variables may make some of the commands called by crmsh to
truncate its output even if those commands are not writing to the
terminal.
These variables is set when bash enables its `checkwinsize` option. As
crmsh is able to read terminal size with curses, these variables are
unneeded and may be out of sync with actual terminal size.
---
crmsh/term.py | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/crmsh/term.py b/crmsh/term.py
index 1ac309dd53..675abd3917 100644
--- a/crmsh/term.py
+++ b/crmsh/term.py
@@ -1,6 +1,6 @@
# Copyright (C) 2008-2011 Dejan Muhamedagic <dmuhamedagic@suse.de>
# See COPYING for license information.
-
+import os
import sys
import re
# from: http://code.activestate.com/recipes/475116/
@@ -151,6 +151,7 @@ def init():
from . import config
if not _term_stream.isatty() and 'color-always' not in config.color.style:
return
+ _ignore_environ()
# Check the terminal type. If we fail, then assume that the
# terminal has no capabilities.
try:
@@ -177,4 +178,12 @@ def is_color(s):
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]
+
# vim:ts=4:sw=4:et:
From 8dde6dbb48582ed40e7099356f39a271a4e07cd8 Mon Sep 17 00:00:00 2001
From: Nicholas Yang <nicholas.yang@suse.com>
Date: Wed, 13 Mar 2024 17:55:26 +0800
Subject: [PATCH 4/5] Fix: pass env to child process explicitly (bsc#1205925)
A bug in python stardard library, python/cpython#100516, make
environment variables, `COLUMNS` and `ROWS` to be added at fork. These
environment variables may make some of the commands called by crmsh to
truncate its output even if those commands are not writing to the
terminal.
Passing argument `env` explicitly is a workaround for the python bug.
---
crmsh/scripts.py | 10 ++++++++--
crmsh/ui_node.py | 11 +++++++----
crmsh/utils.py | 31 +++++++++++++++++++++++++------
crmsh/xmlutil.py | 6 +++++-
4 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/crmsh/scripts.py b/crmsh/scripts.py
index 7add6150c6..3b49c90ed4 100644
--- a/crmsh/scripts.py
+++ b/crmsh/scripts.py
@@ -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
@@ -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)
diff --git a/crmsh/ui_node.py b/crmsh/ui_node.py
index 91a124fa05..05bd8e192d 100644
--- a/crmsh/ui_node.py
+++ b/crmsh/ui_node.py
@@ -1,7 +1,7 @@
# Copyright (C) 2008-2011 Dejan Muhamedagic <dmuhamedagic@suse.de>
# Copyright (C) 2013 Kristoffer Gronlund <kgronlund@suse.com>
# See COPYING for license information.
-
+import os
import re
import copy
import subprocess
@@ -550,10 +550,13 @@ def do_delete(self, context, node):
'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]
+ return 0 == subprocess.call(
+ args,
+ env=os.environ, # bsc#1205925
+ )
@command.wait
@command.completers(compl.nodes, compl.choice(['set', 'delete', 'show']), _find_attr)
diff --git a/crmsh/utils.py b/crmsh/utils.py
index 2e422c009e..8d9adac757 100644
--- a/crmsh/utils.py
+++ b/crmsh/utils.py
@@ -523,7 +523,12 @@ def pipe_string(cmd, s):
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):
@@ -552,7 +557,9 @@ def filter_string(cmd, s, stderr_on=True, shell=True):
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):
@@ -770,7 +777,9 @@ def show_dot_graph(dotfile, keep_file=False, desc="transition graph"):
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)
@@ -779,13 +788,21 @@ def ext_cmd(cmd, shell=True):
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(
+ cmd,
+ shell=shell,
+ env=os.environ, # bsc#1205925
+ )
def rmdir_r(d):
@@ -918,7 +935,9 @@ def pipe_cmd_nosudo(cmd):
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
diff --git a/crmsh/xmlutil.py b/crmsh/xmlutil.py
index ee3f96a4e7..64a088d9ca 100644
--- a/crmsh/xmlutil.py
+++ b/crmsh/xmlutil.py
@@ -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()
From 1ceec363c4ecbc8da6f3f3e449f2482a4f08869b Mon Sep 17 00:00:00 2001
From: Nicholas Yang <nicholas.yang@suse.com>
Date: Wed, 13 Mar 2024 18:47:46 +0800
Subject: [PATCH 5/5] Fix: utils: set env `CIB_shadow` using `os.environ`
(bsc#1205925)
`os.environ` is captured once from libc on python process startup (when
module `os` is imported from `site.py`) and is the only one interface
able to enumerate environment variables. Changing environment variables
with other interfaces (including `os.putenv`, `os.unsetenv`, or call
libc interface in some CPython modules) makes `os.environ` out of sync
and should be avoided.
---
crmsh/utils.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/crmsh/utils.py b/crmsh/utils.py
index 8d9adac757..19fc90cec7 100644
--- a/crmsh/utils.py
+++ b/crmsh/utils.py
@@ -150,13 +150,14 @@ def this_node():
def set_cib_in_use(name):
- os.putenv(_cib_shadow, name)
+ os.environ[_cib_shadow] = name
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]
global _cib_in_use
_cib_in_use = ''