Hi all
There's a bug in the connector API when subsequently adding to and
removing connectors from an event loop.
Here's some dummy code to reproduce it (I will add real code later):
event = ssh_event_new();
/* ADD FIRST connector pair (in/out) */
ssh_connector in1 = ssh_connector_new(session);
ssh_connector_set_out_channel(...);
ssh_connector_set_in_fd(...);
ssh_event_add_connector(event, in1);
ssh_connector out1 = ssh_connector_new(session);
ssh_connector_set_out_fd(...);
....
ssh_event_dopoll(event, 2000);
/* ADD SECOND connector pair */
ssh_connector in2 = ssh_connector_new(session);
ssh_connector_set_out_channel(...);
ssh_connector_set_in_fd(...);
ssh_event_add_connector(event, in2);
ssh_connector out2 = ssh_connector_new(session);
ssh_connector_set_out_fd(...);
....
ssh_event_dopoll(event, 2000);
.... /* work done with connector pair 1 */
/* REMOVE FIRST connector pair */
ssh_event_remove_connector(event, in1);
ssh_event_remove_connector(event, out1);
ssh_connector_free(in1);
ssh_connector_free(in1);
....
ssh_event_dopoll(event, 2000);
/* ADD THIRD connector pair */
ssh_connector in3 = ssh_connector_new(session);
ssh_connector_set_out_channel(...);
ssh_connector_set_in_fd(...);
ssh_event_add_connector(event, in3);
^^^^ ***SIGSEGV ****
The underlying problem is ssh_connector_remove_event() where
connector->in_channel and ->out_channel is NULLed.
The subsequent call to ssh_connector_free() does not remove the
in_channel and out_channel callbacks, because in_ and out_channel is NULL.
So we have some sticky callbacks attached to the channel, and
ssh_connector_remove_event() has removed the poll objects...
Later on, when adding a new connector, ssh_channel_poll_timeout() is
called at a certain point, which calls the CB function with the polling
objects removed.
And voila:
Thread 3 "sshfwd" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff4574700 (LWP 15333)]
0x00007ffff7b76a04 in ssh_poll_get_events (p=0x0) at
/home/till/libssh/src/poll.c:403
403 return p->events;
(gdb) backtrace
#0 0x00007ffff7b76a04 in ssh_poll_get_events (p=0x0) at
/home/till/libssh/src/poll.c:403
#1 0x00007ffff7b76ac3 in ssh_poll_add_events (p=0x0, events=4) at
/home/till/libssh/src/poll.c:443
#2 0x00007ffff7b56cd1 in ssh_connector_reset_pollevents
(connector=0x7fffec0045b0) at /home/till/libssh/src/connector.c:235
#3 0x00007ffff7b5757a in ssh_connector_channel_data_cb
(session=0x7fffec0008c0, channel=0x7fffec006cc0, data=0x7fffec009940, len=120,
is_stderr=0, userdata=0x7fffec0045b0) at
/home/till/libssh/src/connector.c:489
#4 0x00007ffff7b4cf50 in channel_rcv_data (session=0x7fffec0008c0, type=94
'^', packet=0x7fffec001450, user=0x7fffec0008c0)
at /home/till/libssh/src/channels.c:563
#5 0x00007ffff7b6dfb5 in ssh_packet_process (session=0x7fffec0008c0, type=94
'^') at /home/till/libssh/src/packet.c:1421
#6 0x00007ffff7b6d9aa in ssh_packet_socket_callback (data=0x7fffec008d40,
receivedlen=176, user=0x7fffec0008c0)
at /home/till/libssh/src/packet.c:1275
#7 0x00007ffff7b7ba1d in ssh_socket_pollcallback (p=0x7fffec004cc0, fd=6,
revents=1, v_s=0x7fffec001280)
at /home/till/libssh/src/socket.c:302
#8 0x00007ffff7b771ad in ssh_poll_ctx_dopoll (ctx=0x7fffec006900, timeout=-1)
at /home/till/libssh/src/poll.c:702
#9 0x00007ffff7b789eb in ssh_handle_packets (session=0x7fffec0008c0,
timeout=-1) at /home/till/libssh/src/session.c:643
#10 0x00007ffff7b78af1 in ssh_handle_packets_termination
(session=0x7fffec0008c0, timeout=0,
fct=0x7ffff7b502c1 <ssh_channel_read_termination>, user=0x7ffff456fb40) at
/home/till/libssh/src/session.c:711
#11 0x00007ffff7b50867 in ssh_channel_poll_timeout (channel=0x7fffec006cc0,
timeout=0, is_stderr=0)
at /home/till/libssh/src/channels.c:2974
#12 0x00007ffff7b5798c in ssh_connector_set_event (connector=0x7fffec007bb0,
event=0x7fffec0047f0) at /home/till/libssh/src/connector.c:607
#13 0x00007ffff7b7762d in ssh_event_add_connector (event=0x7fffec0047f0,
connector=0x7fffec007bb0) at /home/till/libssh/src/poll.c:937
The big fix is qute simple: Dont' NULL the channel pointers in
ssh_connector_remove_event(). I don't really see the point why they have
to be NULLed there. The event should be removed from the connector, and
not the channels...
Patch attached.
Cheers,
Till
>From e25d985380b6576eed4baee0bd36904a14049266 Mon Sep 17 00:00:00 2001
From: Till Wimmer <twim...@gordiancode.com>
Date: Mon, 28 Jan 2019 23:07:07 +0100
Subject: [PATCH] Don't NULL connector->channels on event remove
Signed-off-by: Till Wimmer <twim...@gordiancode.com>
---
src/connector.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/connector.c b/src/connector.c
index c4f6af5..0062785 100644
--- a/src/connector.c
+++ b/src/connector.c
@@ -641,14 +641,12 @@ int ssh_connector_remove_event(ssh_connector connector) {
session = ssh_channel_get_session(connector->in_channel);
ssh_event_remove_session(connector->event, session);
- connector->in_channel = NULL;
}
if (connector->out_channel != NULL) {
session = ssh_channel_get_session(connector->out_channel);
ssh_event_remove_session(connector->event, session);
- connector->out_channel = NULL;
}
connector->event = NULL;
--
2.7.4
