ssh_channel_accept bug (discards messages incorrectly)

[Eze-DP]

If we are interested in simultaneously accepting multiple different channels, say channel A and channel B (different destination ports), we might have threads iterating and checking the session to see if the channel is ready. However, if we execute the ssh_channel_accept() command for channel B when the next message in the library iterator corresponds to channel A, the library will delete the message in the iterator. I propose that the library first checks if the destination port of the message on the iterator matches the function call, and only if it matches should it execute the remainder of the code. For example:

static ssh_channel ssh_channel_accept(ssh_session session, int channeltype,
    int timeout_ms, int *destination_port, char **originator, int *originator_port)
{
#if !defined( _WIN32) && !defined(ESP32)
  static const struct timespec ts = {
    .tv_sec = 0,
    .tv_nsec = 50000000 /* 50ms */
  };
#endif
  ssh_message msg = NULL;
  ssh_channel channel = NULL;
  struct ssh_iterator *iterator;
  int t;

  /*
   * We sleep for 50 ms in ssh_handle_packets() and later sleep for
   * 50 ms. So we need to decrement by 100 ms.
   */
  int mem = *destination_port;
  for (t = timeout_ms; t >= 0; t -= 100) {
    if (timeout_ms == 0) {
        ssh_handle_packets(session, 0);
    } else {
        ssh_handle_packets(session, 50);
    }

    if (session->ssh_message_list) {
      iterator = ssh_list_get_iterator(session->ssh_message_list);
      while (iterator) {
        msg = (ssh_message)iterator->data;
        if (ssh_message_type(msg) == SSH_REQUEST_CHANNEL_OPEN &&
            ssh_message_subtype(msg) == channeltype && mem == msg->channel_request_open.destination_port) {
          ssh_list_remove(session->ssh_message_list, iterator);
          channel = ssh_message_channel_request_open_reply_accept(msg);
          if(destination_port) {
            *destination_port=msg->channel_request_open.destination_port;
          }
          if(originator) {
            *originator=strdup(msg->channel_request_open.originator);
          }
          if(originator_port) {
            *originator_port=msg->channel_request_open.originator_port;
          }

          //printf("accept successful dstport = %d, oriport = %d, sender = %d, mem = %d \n", msg->channel_request_open.destination_port, msg->channel_request_open.originator_port, msg->channel_request_open.sender, mem);
          ssh_message_free(msg);
          return channel;
        }
        iterator = iterator->next;
      }
    }
    if(t>0){
#ifdef _WIN32
      Sleep(50); /* 50ms */
#elif defined(ESP32)
      vTaskDelay(50 / portTICK_PERIOD_MS); /* 50 ms */
#else
      nanosleep(&ts, NULL);
#endif
    }
  }

So the difference I have made is the addition of the 'mem' variable which is used to ensure messages are only discarded if it is appropriate.

[ewpa]

Hi Ezequiel, would you be able to recreate this unwanted behavior using the libssh.org library on your computer? I think the best place to discuss this is on the libssh mailing list -- unless it is specific to this ESP32 port.

[Eze-DP]

Oops, I brought it here because I was working with this library on an ESP32, but now I have checked the libssh.org repository and have confirmed this issue is present in there as well. I will pass it on to their mailing list as you suggested!