[patch] SIGSEGV when adding Connector to Event

[g4-lisz@xxxxxxxxxxxx]

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 <twimmer@xxxxxxxxxxxxxxx>
Date: Mon, 28 Jan 2019 23:07:07 +0100
Subject: [PATCH] Don't NULL connector->channels on event remove

Signed-off-by: Till Wimmer <twimmer@xxxxxxxxxxxxxxx>
---
 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