open-iscsi / tcmu-runner

A daemon that handles the userspace side of the LIO TCM-User backstore.
Apache License 2.0
190 stars 148 forks source link

Potential error(e.g., deadlock ) due to the unreleased lock (CWE-667) #666

Closed jenny-cheung closed 3 years ago

jenny-cheung commented 3 years ago

Dear developers, thank you for your checking! It seems that the lock &rdev->caw_lock may not be released correctly if the ret == TCMU_STS_ASYNC_HANDLED is true. Should it be a real bug? I think the fix is to insert pthread_mutex_unlock(&rdev->caw_lock); before returning.

https://github.com/open-iscsi/tcmu-runner/blob/06d64ab78c2898c032fe5be93f9ae6f64b199d5b/tcmur_cmd_handler.c#L1731 https://github.com/open-iscsi/tcmu-runner/blob/06d64ab78c2898c032fe5be93f9ae6f64b199d5b/tcmur_cmd_handler.c#L1736

static int handle_caw(struct tcmu_device *dev, struct tcmulib_cmd *cmd)
{
    ...;
    pthread_mutex_lock(&rdev->caw_lock);

    ret = aio_request_schedule(dev, tcmur_cmd, caw_work_fn,
                   tcmur_cmd_complete);
    if (ret == TCMU_STS_ASYNC_HANDLED)
        return TCMU_STS_ASYNC_HANDLED;  //return without releasing the lock

    pthread_mutex_unlock(&rdev->caw_lock);
    tcmur_cmd_state_free(tcmur_cmd);
    return ret;
}

Best,

lxbsz commented 3 years ago

Yeah, it is a bug, could you raise one PR ?

lxbsz commented 3 years ago

Yeah, it is a bug, could you raise one PR ?

Sorry, actually it is not a bug. The unlock will be done in the callbacks.

jenny-cheung commented 3 years ago

@lxbsz OK,Thanks