deadlock on remote_list_mutex

[doadam]

usbmux_remote_process takes a lock on remote_list_mutex when checking for remotes that need to be removed:

void usbmux_remote_process(int fd, short events)
{
    struct remote_mux *remote = NULL;
    pthread_mutex_lock(&remote_list_mutex); // taken
    /* find matching remote, but also reap dead remotes */
    FOREACH(struct remote_mux *rm, &remote_list) {
        if (rm->state == REMOTE_DEAD) {
            usbmux_remote_dispose(rm);
        } else if (rm->fd == fd) {
            remote = rm;
            if (events == POLLNVAL) {
                usbfluxd_log(LL_DEBUG, "%s: remote fd %d became invalid", __func__, fd);
                usbmux_remote_dispose(rm);
                remote = NULL;
            }
        }
    } ENDFOREACH
    pthread_mutex_unlock(&remote_list_mutex); // unlocked

However, usbmux_remote_dispose will ultimately call client_close which takes the lock as well via usbmux_remote_notify_client_close:

void usbmux_remote_dispose(struct remote_mux *remote)
{
    usbfluxd_log(LL_INFO, "%s: Disconnecting remote fd %d", __func__, remote->fd);

    close(remote->fd);

    plist_dict_foreach(remote_device_list, remote_device_notify_remove, (void*)remote);
    collection_remove(&remote_list, remote);
    client_remote_unset(remote);
    if (remote->client) {
        client_notify_remote_close(remote->client); // here
    }
... // more code
}

```c
void client_notify_remote_close(struct mux_client *client)
{
    usbfluxd_log(LL_DEBUG, "%s %p", __func__, client);
    client_close(client);
}

void client_close(struct mux_client *client)
{
    usbfluxd_log(LL_INFO, "Disconnecting client %p fd %d", client, client->fd);
    if(client->state == CLIENT_CONNECTING1 || client->state == CLIENT_CONNECTING2) {
        usbfluxd_log(LL_INFO, "Client died mid-connect, aborting device %d connection", client->connect_device);
        client->state = CLIENT_DEAD;
        //device_abort_connect(client->connect_device, client);
    }
    close(client->fd);
    if (client->remote) {
        usbmux_remote_notify_client_close(client->remote); // this takes the lock
    }
... // more code
}

void usbmux_remote_notify_client_close(struct remote_mux *remote)
{
    pthread_mutex_lock(&remote_list_mutex); // remote_list_mutex is taken again, although it's locked already
    remote_close(remote);
    pthread_mutex_unlock(&remote_list_mutex);
}

This will cause usbfluxd to hang after a while. I fixed it by changing remote_list_mutex to be recursive. A patch is attached as well.

Tested on 0723a9a89a188de0949a6c80177705309ebb21f5, which is master at the time of writing. fix_deadlock.patch

[sbingner]

@doadam can you provide any information on how you duplicated this? It seems to be the same issue as the UAF reported in #8 but I have not been able to duplicate either one, and the logic should have cleared remote->client before

    if (client->remote) {
        usbmux_remote_notify_client_close(client->remote); // this takes the lock
    }

at

    client_remote_unset(remote); <-- *here*
    if (remote->client) {
        client_notify_remote_close(remote->client); // here
    }

at which point this would not be a deadlock

[sbingner]

This should now have the fix verified with doadam in source master (v1.2.1) will be updating binary release soon.