Merge pull request #658 from Jakuje/crash-exec

webknjaz · web-flow · commit baa2ab830d44 · 2025-03-27T17:20:21.000+01:00
Prevent `SEGFAULT`s on consecutive `exec_command()` invocations
diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml
@@ -1101,15 +1101,6 @@ jobs:
           && '4.0.0-2'
           || '2.6.0-1'
         }}${{ steps.distribution-meta.outputs.dist-tag }}.noarch.rpm
-        https://rpmfind.net/linux/epel/"$(
-          rpm --eval '%{rhel}'
-        )"/Everything/x86_64/Packages/p/python3-pytest-forked-${{
-          contains(matrix.target-container.tag, 'ubi9')
-          && '1.4.0'
-          || '1.0.2'
-        }}-1${{
-          steps.distribution-meta.outputs.dist-tag
-        }}.noarch.rpm
         https://rpmfind.net/linux/epel/"$(
           rpm --eval '%{rhel}'
         )"/Everything/x86_64/Packages/p/python3-pytest-xdist-${{
diff --git a/docs/changelog-fragments/658.bugfix.rst b/docs/changelog-fragments/658.bugfix.rst
@@ -0,0 +1 @@
+Repetitive calls to ``exec_channel()`` no longer crash and return reliable output -- by :user:`Jakuje`.
diff --git a/docs/changelog-fragments/658.packaging.rst b/docs/changelog-fragments/658.packaging.rst
@@ -0,0 +1 @@
+The ``pytest-forked`` dependency of build, development and test environments was removed -- by :user:`Jakuje`.
diff --git a/packaging/rpm/ansible-pylibssh.spec b/packaging/rpm/ansible-pylibssh.spec
@@ -56,7 +56,6 @@ Source14: %{pypi_source typing_extensions 4.12.2}
 %endif
 Source15: %{pypi_source pytest 6.2.4}
 Source16: %{pypi_source pytest-cov 2.12.1}
-Source17: %{pypi_source pytest-forked 1.3.0}
 Source18: %{pypi_source pytest-xdist 2.3.0}
 Source19: %{pypi_source iniconfig 1.1.1}
 Source20: %{pypi_source attrs 20.3.0}
@@ -77,7 +76,6 @@ BuildRequires: openssh-clients
 %if 0%{?rhel}
 BuildRequires: python3dist(pytest)
 BuildRequires: python3dist(pytest-cov)
-BuildRequires: python3dist(pytest-forked)
 BuildRequires: python3dist(pytest-xdist)
 BuildRequires: python3dist(tox)
 %endif
@@ -166,8 +164,6 @@ PYTHONPATH="$(pwd)/bin" \
 PYTHONPATH="$(pwd)/bin" \
 %{__python3} -m pip install --no-deps -t bin %{SOURCE16}
 PYTHONPATH="$(pwd)/bin" \
-%{__python3} -m pip install --no-deps -t bin %{SOURCE17}
-PYTHONPATH="$(pwd)/bin" \
 %{__python3} -m pip install --no-deps -t bin %{SOURCE18}
 PYTHONPATH="$(pwd)/bin" \
 %{__python3} -m pip install --no-deps -t bin %{SOURCE19}
diff --git a/pytest.ini b/pytest.ini
@@ -49,7 +49,6 @@ faulthandler_timeout = 30
 filterwarnings =
   error
   ignore:Coverage disabled via --no-cov switch!:pytest.PytestWarning:pytest_cov.plugin
-  ignore:pytest-forked xfail support is incomplete at the moment and may output a misleading reason message:RuntimeWarning:pytest_forked
 
   # FIXME: drop this once `pytest-cov` is updated.
   # Ref: https://github.com/pytest-dev/pytest-cov/issues/557
diff --git a/src/pylibsshext/channel.pxd b/src/pylibsshext/channel.pxd
@@ -23,3 +23,7 @@ cdef class Channel:
     cdef  _session
     cdef libssh.ssh_channel _libssh_channel
     cdef libssh.ssh_session _libssh_session
+
+cdef class ChannelCallback:
+    cdef callbacks.ssh_channel_callbacks_struct callback
+    cdef _userdata
diff --git a/src/pylibsshext/channel.pyx b/src/pylibsshext/channel.pyx
@@ -45,6 +45,16 @@ cdef int _process_outputs(libssh.ssh_session session,
         result.stdout += data_b
     return len
 
+cdef class ChannelCallback:
+    def __cinit__(self):
+        memset(&self.callback, 0, sizeof(self.callback))
+        callbacks.ssh_callbacks_init(&self.callback)
+        self.callback.channel_data_function = <callbacks.ssh_channel_data_callback>&_process_outputs
+
+    def set_user_data(self, userdata):
+        self._userdata = userdata
+        self.callback.userdata = <void *>self._userdata
+
 cdef class Channel:
     def __cinit__(self, session):
         self._session = session
@@ -159,19 +169,23 @@ cdef class Channel:
             libssh.ssh_channel_free(channel)
             raise LibsshChannelException("Failed to open_session: [{0}]".format(rc))
 
+        result = CompletedProcess(args=command, returncode=-1, stdout=b'', stderr=b'')
+
+        cb = ChannelCallback()
+        cb.set_user_data(result)
+        callbacks.ssh_set_channel_callbacks(channel, &cb.callback)
+        # keep the callback around in the session object to avoid use after free
+        self._session.push_callback(cb)
+
         rc = libssh.ssh_channel_request_exec(channel, command.encode("utf-8"))
         if rc != libssh.SSH_OK:
             libssh.ssh_channel_close(channel)
             libssh.ssh_channel_free(channel)
             raise LibsshChannelException("Failed to execute command [{0}]: [{1}]".format(command, rc))
-        result = CompletedProcess(args=command, returncode=-1, stdout=b'', stderr=b'')
 
-        cdef callbacks.ssh_channel_callbacks_struct cb
-        memset(&cb, 0, sizeof(cb))
-        cb.channel_data_function = <callbacks.ssh_channel_data_callback>&_process_outputs
-        cb.userdata = <void *>result
-        callbacks.ssh_callbacks_init(&cb)
-        callbacks.ssh_set_channel_callbacks(channel, &cb)
+        # wait before remote writes all data before closing the channel
+        while not libssh.ssh_channel_is_eof(channel):
+            libssh.ssh_channel_poll(channel, 0)
 
         libssh.ssh_channel_send_eof(channel)
         result.returncode = libssh.ssh_channel_get_exit_status(channel)
diff --git a/src/pylibsshext/session.pxd b/src/pylibsshext/session.pxd
@@ -26,5 +26,6 @@ cdef class Session:
     cdef _hash_py
     cdef _fingerprint_py
     cdef _keytype_py
+    cdef _channel_callbacks
 
 cdef libssh.ssh_session get_libssh_session(Session session)
diff --git a/src/pylibsshext/session.pyx b/src/pylibsshext/session.pyx
@@ -108,6 +108,10 @@ cdef class Session(object):
         self._hash_py = None
         self._fingerprint_py = None
         self._keytype_py = None
+        # Due to delayed freeing of channels, some older libssh versions might expect
+        # the callbacks to be around even after we free the underlying channels so
+        # we should free them only when we terminate the session.
+        self._channel_callbacks = []
 
     def __cinit__(self, host=None, **kwargs):
         self._libssh_session = libssh.ssh_new()
@@ -124,6 +128,9 @@ cdef class Session(object):
             libssh.ssh_free(self._libssh_session)
             self._libssh_session = NULL
 
+    def push_callback(self, callback):
+        self._channel_callbacks.append(callback)
+
     @property
     def port(self):
         cdef unsigned int port_i
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -3,6 +3,7 @@
 
 """Pytest plugins and fixtures configuration."""
 
+import logging
 import shutil
 import socket
 import subprocess
@@ -115,6 +116,8 @@ def ssh_client_session(ssh_session_connect):
     # noqa: DAR101
     """
     ssh_session = Session()
+    # TODO Adjust when #597 will be merged
+    ssh_session.set_log_level(logging.CRITICAL)
     ssh_session_connect(ssh_session)
     try:  # noqa: WPS501
         yield ssh_session
@@ -173,6 +176,7 @@ def sshd_addr(free_port_num, ssh_authorized_keys_path, sshd_hostkey_path, sshd_p
         '/usr/sbin/sshd',
         '-D',
         '-f', '/dev/null',
+        '-E', '/dev/stderr',
         opt, 'LogLevel=DEBUG3',
         opt, 'HostKey={key!s}'.format(key=sshd_hostkey_path),
         opt, 'PidFile={pid!s}'.format(pid=sshd_path / 'sshd.pid'),
@@ -187,7 +191,6 @@ def sshd_addr(free_port_num, ssh_authorized_keys_path, sshd_hostkey_path, sshd_p
         opt, 'StrictModes=no',
         opt, 'PermitEmptyPasswords=yes',
         opt, 'PermitRootLogin=yes',
-        opt, 'Protocol=2',
         opt, 'HostbasedAuthentication=no',
         opt, 'IgnoreUserKnownHosts=yes',
         opt, 'Port={port:d}'.format(port=free_port_num),  # port before addr
diff --git a/tests/unit/channel_test.py b/tests/unit/channel_test.py
@@ -33,19 +33,36 @@ def ssh_channel(ssh_client_session):
         chan.close()
 
 
-@pytest.mark.xfail(
-    reason='This test causes SEGFAULT, flakily. '
-    'Ref: https://github.com/ansible/pylibssh/issues/57',
-    strict=False,
-)
-@pytest.mark.forked
+def exec_second_command(ssh_channel):
+    """Check the standard output of ``exec_command()`` as a string."""
+    u_cmd = ssh_channel.exec_command('echo -n Hello Again')
+    assert u_cmd.returncode == 0
+    assert u_cmd.stderr.decode() == ''  # noqa: WPS302
+    assert u_cmd.stdout.decode() == u'Hello Again'  # noqa: WPS302
+
+
 def test_exec_command(ssh_channel):
     """Test getting the output of a remotely executed command."""
-    u_cmd_out = ssh_channel.exec_command('echo -n Hello World').stdout.decode()
-    assert u_cmd_out == u'Hello World'  # noqa: WPS302
+    u_cmd = ssh_channel.exec_command('echo -n Hello World')
+    assert u_cmd.returncode == 0
+    assert u_cmd.stderr.decode() == ''
+    assert u_cmd.stdout.decode() == u'Hello World'  # noqa: WPS302
     # Test that repeated calls to exec_command do not segfault.
-    u_cmd_out = ssh_channel.exec_command('echo -n Hello Again').stdout.decode()
-    assert u_cmd_out == u'Hello Again'  # noqa: WPS302
+
+    # NOTE: Call `exec_command()` once again from another function to
+    # NOTE: force it to happen in another place of the call stack,
+    # NOTE: making sure that the context is different from one in this
+    # NOTE: this test function. The resulting call stack will end up
+    # NOTE: being more random.
+    exec_second_command(ssh_channel)
+
+
+def test_exec_command_stderr(ssh_channel):
+    """Test getting the stderr of a remotely executed command."""
+    u_cmd = ssh_channel.exec_command('echo -n Hello World 1>&2')
+    assert u_cmd.returncode == 0
+    assert u_cmd.stderr.decode() == u'Hello World'  # noqa: WPS302
+    assert u_cmd.stdout.decode() == ''
 
 
 def test_double_close(ssh_channel):
diff --git a/tox.ini b/tox.ini
@@ -18,7 +18,6 @@ deps =
   coverage >= 5.3
   pytest
   pytest-cov
-  pytest-forked
   pytest-xdist
 passenv =
   PYTHONPATH
@@ -44,7 +43,6 @@ deps =
   expandvars
   pytest
   pytest-cov
-  pytest-forked
   pytest-xdist
 setenv =
   ANSIBLE_PYLIBSSH_CYTHON_TRACING = {env:ANSIBLE_PYLIBSSH_CYTHON_TRACING:1}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Repetitive calls to ``exec_channel()`` no longer crash and return reliable output -- by :user:`Jakuje`.
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+The ``pytest-forked`` dependency of build, development and test environments was removed -- by :user:`Jakuje`.