Am 14.03.2019 um 18:27 hat Peter Maydell geschrieben:
> On Tue, 12 Mar 2019 at 17:30, Kevin Wolf <kw...@redhat.com> 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.
raw_reconfigure_getfd() returns a file descriptor that works for the
given parameters. This can be the existing one (the ret == s->fd case) or
a new one. We only own the reference and need to store it if it's a new
one.
So yes, I think it is a false positive.
Kevin
