Re: [Qemu-block] [PULL 13/28] file-posix: Prepare permission code for fd switching

[Peter Maydell]

On Tue, 12 Mar 2019 at 17:30, Kevin Wolf <address@hidden> wrote:
>
> In order to be able to dynamically reopen the file read-only or
> read-write, depending on the users that are attached, we need to be able
> to switch to a different file descriptor during the permission change.
>
> This interacts with reopen, which also creates a new file descriptor and
> performs permission changes internally. In this case, the permission
> change code must reuse the reopen file descriptor instead of creating a
> third one.
>
> In turn, reopen can drop its code to copy file locks to the new file
> descriptor because that is now done when applying the new permissions.

Hi -- Coverity doesn't like this function (CID 1399712).
I think this may be a false positive, but could you confirm?

> @@ -2696,12 +2695,78 @@ static QemuOptsList raw_create_opts = {
>  static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t 
> shared,
>                            Error **errp)
>  {
> -    return raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
> +    BDRVRawState *s = bs->opaque;
> +    BDRVRawReopenState *rs = NULL;
> +    int open_flags;
> +    int ret;
> +
> +    if (s->perm_change_fd) {
> +        /*
> +         * In the context of reopen, this function may be called several 
> times
> +         * (directly and recursively while change permissions of the parent).
> +         * This is even true for children that don't inherit from the 
> original
> +         * reopen node, so s->reopen_state is not set.
> +         *
> +         * Ignore all but the first call.
> +         */
> +        return 0;
> +    }
> +
> +    if (s->reopen_state) {
> +        /* We already have a new file descriptor to set permissions for */
> +        assert(s->reopen_state->perm == perm);
> +        assert(s->reopen_state->shared_perm == shared);
> +        rs = s->reopen_state->opaque;
> +        s->perm_change_fd = rs->fd;
> +    } else {
> +        /* We may need a new fd if auto-read-only switches the mode */
> +        ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags,
> +                                    false, errp);

Coverity says that raw_reconfigure_getfd() returns an fd in 'ret' here...

> +        if (ret < 0) {
> +            return ret;
> +        } else if (ret != s->fd) {
> +            s->perm_change_fd = ret;
> +        }
> +    }
> +
> +    /* Prepare permissions on old fd to avoid conflicts between old and new,
> +     * but keep everything locked that new will need. */
> +    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);

...but this call overwrites that fd, so we might never close it.

I think the answer is that either:
 * ret == s->fd and we'll close s->fd later
 * or we save ret into s->perm_change_fd

and Coverity just isn't clever enough to realise that if
ret == s->fd then we haven't lost the handle.

Is that right? If so I'll mark it as a false-positive in the UI.

thanks
-- PMM