Page MenuHomePhabricator

Unable to distinguish between real error and child-process interrupts when ssh_bind_accept() returns
Open, Needs TriagePublic

Description

As discussed on #libssh IRC channel, I am calling ssh_bind_accept() - it returns in the case of an error, but also in the case of signals passed by a child process ending. We are unable to distinguish between errors and other signals, because in all cases the code returned by ssh_get_error_code() is SSH_FATAL. I think that, if the underlying accept() call fails and sets an errno of 'EINTR', ssh_bind_accept should ssh_set_error with SSH_EINTR so the caller can distinguish between the different events.

Event Timeline

orionltd created this task.Apr 6 2020, 5:24 PM

Sorry, I can't create a proper PR at the moment (permissions problems and the 'create merge request' by email likewise), and I've not been able to test-compile this code on the latest version - but I have confirmed the same fix works on 0.7.5.

I hope that's enough for now, but I'll follow up with you in the next few days so please do ask if you need more information.

My proposed new code for ssh_bind_accept is as follows:

int ssh_bind_accept(ssh_bind sshbind, ssh_session session) {
  socket_t fd = SSH_INVALID_SOCKET;
  int rc;
  if (sshbind->bindfd == SSH_INVALID_SOCKET) {
    ssh_set_error(sshbind, SSH_FATAL,
        "Can't accept new clients on a not bound socket.");
    return SSH_ERROR;
  }

  if (session == NULL){
      ssh_set_error(sshbind, SSH_FATAL,"session is null");
      return SSH_ERROR;
  }

  fd = accept(sshbind->bindfd, NULL, NULL);
  if (fd == SSH_INVALID_SOCKET) {
    if (errno == EINTR) {
      ssh_set_error(sshbind, SSH_EINTR,
          "Accepting a new connection (child signal error): %s",
          strerror(errno));
      }
    else {
      ssh_set_error(sshbind, SSH_FATAL,
          "Accepting a new connection: %s",
          strerror(errno));
    }
    return SSH_ERROR;
  }
  rc = ssh_bind_accept_fd(sshbind, session, fd);

  if(rc == SSH_ERROR){
      CLOSE_SOCKET(fd);
      ssh_socket_free(session->socket);
  }
  return rc;
}
asn assigned this task to ansasaki.Apr 7 2020, 8:08 AM
Jakuje added a subscriber: Jakuje.Apr 9 2020, 12:26 PM
In T224#3742, @orionltd wrote:
fd = accept(sshbind->bindfd, NULL, NULL);
if (fd == SSH_INVALID_SOCKET) {
  if (errno == EINTR) {
    ssh_set_error(sshbind, SSH_EINTR,
        "Accepting a new connection (child signal error): %s",
        strerror(errno));

It looks like the SSH_EINTR is not used anywhere in the code yet so I am not sure how appropriate is to set new errors with this type.

else {
  ssh_set_error(sshbind, SSH_FATAL,
      "Accepting a new connection: %s",
      strerror(errno));
}
return SSH_ERROR;

Additionally, I would consider returning different value than SSH_ERROR. I am not sure if SSH_AGAIN would be appropriate, but it is probably closest we have.