>>On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote:
>> This commit fixes the following warning when building virtio_snd driver.
>>
>> "
>> *** CID 1583619: Uninitialized variables (UNINIT)
>> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op()
>> 288
>> 289 break;
>> 290 }
>> 291
>> 292 kfree(tlv);
>> 293
>> vvv CID 1583619: Uninitialized variables (UNINIT)
>> vvv Using uninitialized value "rc".
>> 294 return rc;
>> 295 }
>> 296
>> 297 /**
>> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type.
>> 299 * @snd: VirtIO sound device.
>> "
>>
>> This warning is caused by the absence of the "default" branch in the
>> switch-block, and is a false positive because the kernel calls
>> virtsnd_kctl_tlv_op() only with values for op_flag processed in
>> this block.
>>
>> Also, this commit unifies the cleanup path for all possible control
>> paths in the callback function.
>>
>> Signed-off-by: Anton Yakovlev <anton.yakovlev@xxxxxxxxxxxxxxx>
>> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@xxxxxxxxxxxxxxx>
>> Reported-by: coverity-bot <keescook+coverity-bot@xxxxxxxxxxxx>
>> Addresses-Coverity-ID: 1583619 ("Uninitialized variables")
>> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls")
>> ---
>> sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++----
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
>> index 0c6ac74aca1e..40606eb381af 100644
>> --- a/sound/virtio/virtio_kctl.c
>> +++ b/sound/virtio/virtio_kctl.c
>> @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
>>
>> tlv = kzalloc(size, GFP_KERNEL);
>> if (!tlv) {
>> - virtsnd_ctl_msg_unref(msg);
>> - return -ENOMEM;
>> + rc = -ENOMEM;
>> + goto on_cleanup;
>> }
>>
>> sg_init_one(&sg, tlv, size);
>> @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
>> case SNDRV_CTL_TLV_OP_READ:
>> hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);
>>
>> + /* Since virtsnd_ctl_msg_send() drops the reference, we increase
>> + * the counter to be consistent with the on_cleanup path.
>> + */
> This is not how multi-line comments should look.
> Adding overhead here is just a waste of cycles.
> Instead, separate error handling and normal exit paths.
> Then you will not need to increase the refcount here.
Ok.
>> + virtsnd_ctl_msg_ref(msg);
>> +
>> rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
>> if (!rc) {
>> if (copy_to_user(utlv, tlv, size))
>> @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
>> hdr->hdr.code =
>> cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
>>
>> - if (copy_from_user(tlv, utlv, size))
>> + if (copy_from_user(tlv, utlv, size)) {
>> rc = -EFAULT;
>> - else
>> + } else {
>> + /* Same as the comment above */
> Same thing.
> Besides, this kind of cross referencing breaks immediately when
> someone adds a comment in the middle.
I agree. I will update the patch
>> + virtsnd_ctl_msg_ref(msg);
>> +
>> rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
>> + }
>> +
>> + break;
>> + default:
>> + rc = -EINVAL;
> /* We never get here - we listed all values for op_flag */
>> + WARN_ON(1);
>>
>> break;
>> }
>>
>> +on_cleanup:
>> + virtsnd_ctl_msg_unref(msg);
>> +
>> kfree(tlv);
>>
>> return rc;
> on_cleanup is not informative, coding style says:
> "Choose label names which say what the goto does or why the goto
> exists."
> And saving on duplication here by paying elsewhere does not make sense.
> So you do this instead:
> kfree(tlv);
> return rc;
> on_error:
> virtsnd_ctl_msg_unref(msg);
> kfree(tlv);
> return rc;
> This is very ideomatic.
Thanks. I will separate error and non error cases and update patch
Thanks,
Aiswarya Cyriac
Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
EMail: aiswarya.cyriac@xxxxxxxxxxxxxxx
www.opensynergy.com
Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Régis Adjamah
________________________________________
From: Michael S. Tsirkin <mst@xxxxxxxxxx>
Sent: Wednesday, February 14, 2024 4:36 PM
To: Aiswarya Cyriac
Cc: jasowang@xxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; virtio-dev@xxxxxxxxxxxxxxxxxxxx; Anton Yakovlev; coverity-bot
Subject: Re: [v3 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.
On Wed, Feb 14, 2024 at 03:01:10PM +0100, Aiswarya Cyriac wrote:
> This commit fixes the following warning when building virtio_snd driver.
>
> "
> *** CID 1583619: Uninitialized variables (UNINIT)
> sound/virtio/virtio_kctl.c:294 in virtsnd_kctl_tlv_op()
> 288
> 289 break;
> 290 }
> 291
> 292 kfree(tlv);
> 293
> vvv CID 1583619: Uninitialized variables (UNINIT)
> vvv Using uninitialized value "rc".
> 294 return rc;
> 295 }
> 296
> 297 /**
> 298 * virtsnd_kctl_get_enum_items() - Query items for the ENUMERATED element type.
> 299 * @snd: VirtIO sound device.
> "
>
> This warning is caused by the absence of the "default" branch in the
> switch-block, and is a false positive because the kernel calls
> virtsnd_kctl_tlv_op() only with values for op_flag processed in
> this block.
>
> Also, this commit unifies the cleanup path for all possible control
> paths in the callback function.
>
> Signed-off-by: Anton Yakovlev <anton.yakovlev@xxxxxxxxxxxxxxx>
> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@xxxxxxxxxxxxxxx>
> Reported-by: coverity-bot <keescook+coverity-bot@xxxxxxxxxxxx>
> Addresses-Coverity-ID: 1583619 ("Uninitialized variables")
> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls")
> ---
> sound/virtio/virtio_kctl.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> index 0c6ac74aca1e..40606eb381af 100644
> --- a/sound/virtio/virtio_kctl.c
> +++ b/sound/virtio/virtio_kctl.c
> @@ -253,8 +253,8 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
>
> tlv = kzalloc(size, GFP_KERNEL);
> if (!tlv) {
> - virtsnd_ctl_msg_unref(msg);
> - return -ENOMEM;
> + rc = -ENOMEM;
> + goto on_cleanup;
> }
>
> sg_init_one(&sg, tlv, size);
> @@ -266,6 +266,11 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> case SNDRV_CTL_TLV_OP_READ:
> hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);
>
> + /* Since virtsnd_ctl_msg_send() drops the reference, we increase
> + * the counter to be consistent with the on_cleanup path.
> + */
This is not how multi-line comments should look.
Adding overhead here is just a waste of cycles.
Instead, separate error handling and normal exit paths.
Then you will not need to increase the refcount here.
> + virtsnd_ctl_msg_ref(msg);
> +
> rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
> if (!rc) {
> if (copy_to_user(utlv, tlv, size))
> @@ -281,14 +286,26 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
> hdr->hdr.code =
> cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
>
> - if (copy_from_user(tlv, utlv, size))
> + if (copy_from_user(tlv, utlv, size)) {
> rc = -EFAULT;
> - else
> + } else {
> + /* Same as the comment above */
Same thing.
Besides, this kind of cross referencing breaks immediately when
someone adds a comment in the middle.
> + virtsnd_ctl_msg_ref(msg);
> +
> rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
> + }
> +
> + break;
> + default:
> + rc = -EINVAL;
/* We never get here - we listed all values for op_flag */
> + WARN_ON(1);
>
> break;
> }
>
> +on_cleanup:
> + virtsnd_ctl_msg_unref(msg);
> +
> kfree(tlv);
>
> return rc;
on_cleanup is not informative, coding style says:
"Choose label names which say what the goto does or why the goto
exists."
And saving on duplication here by paying elsewhere does not make sense.
So you do this instead:
kfree(tlv);
return rc;
on_error:
virtsnd_ctl_msg_unref(msg);
kfree(tlv);
return rc;
This is very ideomatic.
> --
> 2.43.0
