Re: [PATCH] server: Add a ssh_send_keepalive() function

[Nicolas Viennot <nicolas@xxxxxxxxxxx>]

I'm sending a revised patch to address your concerns.

The libssh client respond with the following when receiving some keepalives:

  [tmate] [3] [ssh_socket_unbuffered_write]
ssh_socket_unbuffered_write: Enabling POLLOUT for socket
  [tmate] [3] [ssh_packet_socket_callback] ssh_packet_socket_callback:
packet: read type 80   [len=44,padding=15,comp=28,payload=27]
  [tmate] [3] [ssh_packet_process] ssh_packet_process: Dispatching
handler for packet type 80
  [tmate] [1] [ssh_packet_process] ssh_packet_process: Couldn't do
anything with packet type 80
  [tmate] [3] [packet_send2] packet_send2: packet: wrote
[len=12,padding=5,comp=6,payload=5]
  [tmate] [3] [ssh_socket_unbuffered_write]
ssh_socket_unbuffered_write: Enabling POLLOUT for socket

We don't care about the response, as it's just used to keep the
connection alive, so it's okay.

Further, clients should be tolerant to getting such keepalive
requests, as real openssh servers sends these sorts of things.

On an unrelated note, the legacy session logging callbacks
(log_function in ssh_callbacks_struct) is broken when using multiple
ssh sessions, while some get ssh_free()ed. I got a bunch of segfaults
due the hacks done in ssh_legacy_log_callback().

Also, if you want to add the http://tmate.io/ project to the list of
"Who uses libssh?" I wouldn't mind ;)

Have a good one,
Nico.

On Sun, Nov 3, 2013 at 5:05 AM, Andreas Schneider <asn@xxxxxxxxxxxxxx> wrote:
> On Saturday 02 November 2013 22:33:42 Nicolas Viennot wrote:
>> Implementation of a ssh_send_keepalive() function.
>> Useful for keeping some client around as NATs may terminate connections.
>
> Hi Nicolas,
>
> did you check if/how the replies are handled? Also how do we find out if the
> client can handle it?
>
> I would also see come code refactoring.
>
>> Signed-off-by: Nicolas Viennot <nicolas@xxxxxxxxxxx>
>> ---
>>  include/libssh/server.h |  2 ++
>>  src/server.c            | 34 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/include/libssh/server.h b/include/libssh/server.h
>> index dbffecd..9d095fe 100644
>> --- a/include/libssh/server.h
>> +++ b/include/libssh/server.h
>> @@ -382,6 +382,8 @@ LIBSSH_API int ssh_channel_write_stderr(ssh_channel
>> channel, const void *data, uint32_t len);
>>
>> +LIBSSH_API int ssh_send_keepalive(ssh_session session);
>> +
>>  /* deprecated functions */
>>  SSH_DEPRECATED LIBSSH_API int ssh_accept(ssh_session session);
>>  SSH_DEPRECATED LIBSSH_API int channel_write_stderr(ssh_channel channel,
>> diff --git a/src/server.c b/src/server.c
>> index 0b62776..b31371f 100644
>> --- a/src/server.c
>> +++ b/src/server.c
>> @@ -1221,6 +1221,40 @@ int ssh_execute_message_callbacks(ssh_session
>> session){ return SSH_OK;
>>  }
>>
>> +
>> +int ssh_send_keepalive(ssh_session session)
>> +{
>> +  struct ssh_string_struct *req;
>> +  int reply = 1;
>> +  int rc = SSH_ERROR;
>> +
>> +  req = ssh_string_from_char("keepalive@xxxxxxxxxxx");
>> +  if (req == NULL) {
>> +    ssh_set_error_oom(session);
>> +    goto out;
>> +  }
>> +
>> +  if (buffer_add_u8(session->out_buffer, SSH2_MSG_GLOBAL_REQUEST) < 0 ||
>> +      buffer_add_ssh_string(session->out_buffer, req) < 0 ||
>> +      buffer_add_u8(session->out_buffer, reply == 0 ? 0 : 1) < 0) {
>> +    ssh_set_error_oom(session);
>> +    goto out;
>> +  }
>
> I would like this to be more like:
>
>      rc = buffer_add_u8(session->out_buffer, SSH2_MSG_GLOBAL_REQUEST);
>      if (rc < 0) {
>          ssh_set_error_oom(session);
>                   goto error;
>      }
>
>      req = ssh_string_from_char("keepalive@xxxxxxxxxxx");
>      if (req == NULL) {
>          ssh_set_error_oom(session);
>          goto error;
>      }
>
>      rc =  buffer_add_ssh_string(session->out_buffer, req);
>      string_free(req);
>      if (rc < 0) {
>          ssh_set_error_oom(session);
>          goto error;
>      }
>
>         ...
>
> error:
>      buffer_reinit(session->out_buffer);
>      return SSH_ERROR;
>
> This way the code is easier to debug.
>
> gdb$ print rc
>
>> +
>> +  if (packet_send(session) == SSH_ERROR)
>> +    goto out;
>
> Same here.
>
>      rc = packet_send(session);
>      if (rc == SSH_ERROR) {
>          goto out;
>      }
>
> Please always add brackets. We learned it the hard way in Samba. We had too
> many bugs cause of missing brackets.
>
>> +
>> +  ssh_handle_packets(session, 0);
>> +
>> +  SSH_LOG(SSH_LOG_PACKET, "Sent a keepalive");
>> +  rc = SSH_OK;
>> +
>> +out:
>> +  ssh_string_free(req);
>> +  return rc;
>> +}
>> +
>>  /** @} */
>>
>>  /* vim: set ts=4 sw=4 et cindent: */
>
>
>