Re: [patch] SIGSEGV when adding Connector to Event

[g4-lisz@xxxxxxxxxxxx]

Hi Andreas,

did you test it? It would be great if this patch could go to the lib. At
the moment I have to statically link my ssh app with my own libssh
version...

Cheers,
Till

On 30.01.19 15:33, g4-lisz@xxxxxxxxxxxx wrote:
>
> Hi all,
>
> You finally find the test code attached. It's a bit complicated, but
> it can reproduce the issue:
>
> 1. Connect to an ssh server which supports direct socket tunnelling:
>
> > ./multichannelfw root myserver.com 22
>
> This opens a local listening port at 20021
>
> 2. Open two or more simultaneous ssh sessions via tunnel:
>
> > ssh root@localhost -p20021
>
> 3. Close one session
>
> 4. Open a new one (will stall)
>
> 5. Trigger some traffic by typing something in an already open session*
>
> 6. Segmentation fault
>
> * This is also affected by my other patch
> "ssh_handle_packets_termination() ignores timeout=0". With this patch,
> there's no need to trigger traffic to unblock receving messages)
>
> Regards,
> Till
>
> On 28.01.19 23:10, g4-lisz@xxxxxxxxxxxx wrote:
>>
>> 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
>>