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.

Sign up for GitHub

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

Jump to bottom

Prevent SEGFAULTs on consecutive exec_command() invocations #658

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from
Open

Prevent SEGFAULTs on consecutive exec_command() invocations #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
398e3af
channels: Use dynamically allocated memory for callbacks
Jakuje Nov 14, 2024
f0a908a
tests: Adjusted reproducer for #57
Jakuje Nov 14, 2024
805f0a0
Add changelog
Jakuje Nov 14, 2024
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 docs/changelog-fragments/658.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Repetitive calls to ``exec_channel()`` no longer crash -- by :user:`Jakuje`.
13 changes: 9 additions & 4 deletions src/pylibsshext/channel.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import time
from io import BytesIO

from cpython.bytes cimport PyBytes_AS_STRING
from cpython.mem cimport PyMem_Malloc
from libc.string cimport memset

from pylibsshext.errors cimport LibsshChannelException
Expand Down Expand Up @@ -166,19 +167,23 @@ cdef class 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_size = sizeof(callbacks.ssh_channel_callbacks_struct)
cdef callbacks.ssh_channel_callbacks_struct *cb = <callbacks.ssh_channel_callbacks_struct *>PyMem_Malloc(cb_size)
if cb is NULL:
raise LibsshChannelException("Memory allocation error")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov says this line is uncovered: https://app.codecov.io/gh/ansible/pylibssh/pull/658#17ffa324129dc304109f4a66c79769d7-R173.

Could you add a test covering this newly added line so that all the new lines in the patch are covered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a simple way to make the malloc fail to add to the test coverage?

memset(cb, 0, cb_size)
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)
callbacks.ssh_callbacks_init(cb)
callbacks.ssh_set_channel_callbacks(channel, cb)

libssh.ssh_channel_send_eof(channel)
result.returncode = libssh.ssh_channel_get_exit_status(channel)
if channel is not NULL:
libssh.ssh_channel_close(channel)
libssh.ssh_channel_free(channel)

# XXX leaking the memory of the callbacks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on what this means? Is this a FIXME? Does it have to remain in the patch? Why not use a # FIXME: comment instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is a side-effect that might be considered as a bug, I would prefer to keep a reference to that. Other option might be to keep some list of callbacks on the session layer to free when we free session, but I wanted first PoC to confirm this approach was working. This will be an issue for older libssh versions before we will fix it with the following change in libssh: https://gitlab.com/libssh/libssh-mirror/-/merge_requests/549

Given that I see this works, I can probably implement more cleaner approach without memory leaks, but probably only tomorrow.

return result

def send_eof(self):
Expand Down
16 changes: 9 additions & 7 deletions tests/unit/channel_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,21 @@ 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,
)
def exec_second_command(ssh_channel):
"""Call exec_command() from different context in the call stack."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling from a different context is a part of the motivation. Within this function, there's nothing that changes the context. You're probably referring to the calling test but this is not something that would go into the docstring because it's not what the function does, it's what that test does, probably.

Copy link
Member

@webknjaz webknjaz Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Call exec_command() from different context in the call stack."""
"""Return ``exec_command()`` stdout as a text string."""

u_cmd_out = ssh_channel.exec_command('echo -n Hello Again').stdout.decode()
assert u_cmd_out == u'Hello Again' # noqa: WPS302


@pytest.mark.forked
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
# 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

# randomize the stack a bit more by calling this from yet another function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this accurate enough?

Suggested change
# randomize the stack a bit more by calling this from yet another function
# 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_double_close(ssh_channel):
Expand Down
Loading