Re: [PULL 03/11] tools: build qemu-vmsr-helper

[Peter Maydell]

On Tue, 23 Jul 2024 at 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Anthony Harivel <aharivel@redhat.com>
>
> Introduce a privileged helper to access RAPL MSR.

Hi; Coverity points out an issue with this commit
(CID 1558555):


> +static void coroutine_fn vh_co_entry(void *opaque)
> +{
> +    VMSRHelperClient *client = opaque;
> +    Error *local_err = NULL;
> +    unsigned int peer_pid;
> +    uint32_t request[3];
> +    uint64_t vmsr;
> +    int r;
> +
> +    qio_channel_set_blocking(QIO_CHANNEL(client->ioc),
> +                             false, NULL);
> +
> +    qio_channel_set_follow_coroutine_ctx(QIO_CHANNEL(client->ioc), true);
> +
> +    /*
> +     * Check peer credentials
> +     */
> +    r = qio_channel_get_peerpid(QIO_CHANNEL(client->ioc),
> +                                &peer_pid,
> +                                &local_err);
> +    if (r < 0) {
> +        error_report_err(local_err);
> +        goto out;

Here we have a check for r < 0 that forces an early exit...

> +    }
> +
> +    while (r < 0) {

...but then immediately we do a while (r < 0). r cannot be < 0
here because we just checked that, so this while loop will
never execute and the whole loop body is dead code.

What was the intention here ?


> +        /*
> +         * Read the requested MSR
> +         * Only RAPL MSR in rapl-msr-index.h is allowed
> +         */
> +        r = qio_channel_read_all(QIO_CHANNEL(client->ioc),
> +                                (char *) &request, sizeof(request), 
> &local_err);
> +        if (r < 0) {
> +            error_report_err(local_err);
> +            break;
> +        }
> +
> +        if (!is_msr_allowed(request[0])) {
> +            error_report("Requested unallowed msr: %d", request[0]);
> +            break;
> +        }
> +
> +        vmsr = vmsr_read_msr(request[0], request[1]);
> +
> +        if (!is_tid_present(peer_pid, request[2])) {
> +            error_report("Requested TID not in peer PID: %d %d",
> +                peer_pid, request[2]);
> +            vmsr = 0;
> +        }
> +
> +        r = qio_channel_write_all(QIO_CHANNEL(client->ioc),
> +                                  (char *) &vmsr,
> +                                  sizeof(vmsr),
> +                                  &local_err);
> +        if (r < 0) {
> +            error_report_err(local_err);
> +            break;
> +        }
> +    }
> +out:
> +    object_unref(OBJECT(client->ioc));
> +    g_free(client);
> +}

thanks
-- PMM