* [virtio-dev] [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.
@ 2024-02-16 10:06 Aiswarya Cyriac
2024-02-16 11:27 ` [virtio-dev] " Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Aiswarya Cyriac @ 2024-02-16 10:06 UTC (permalink / raw)
To: mst, jasowang, perex, tiwai, linux-kernel, alsa-devel,
virtualization, virtio-dev
Cc: Aiswarya Cyriac, Anton Yakovlev, coverity-bot
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@opensynergy.com>
Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com>
Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1583619 ("Uninitialized variables")
Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls")
---
sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
index 0c6ac74aca1e..7aa79c05b464 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_msg_unref;
}
sg_init_one(&sg, tlv, size);
@@ -281,14 +281,25 @@ 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
+ goto on_msg_unref;
+ } else {
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);
+ goto on_msg_unref;
}
+ kfree(tlv);
+ return rc;
+on_msg_unref:
+ virtsnd_ctl_msg_unref(msg);
kfree(tlv);
return rc;
--
2.43.2
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [virtio-dev] Re: [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.
2024-02-16 10:06 [virtio-dev] [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning Aiswarya Cyriac
@ 2024-02-16 11:27 ` Michael S. Tsirkin
[not found] ` <87eddcikz6.wl-tiwai@suse.de>
0 siblings, 1 reply; 3+ messages in thread
From: Michael S. Tsirkin @ 2024-02-16 11:27 UTC (permalink / raw)
To: Aiswarya Cyriac
Cc: jasowang, perex, tiwai, linux-kernel, alsa-devel, virtualization,
virtio-dev, Anton Yakovlev, coverity-bot
On Fri, Feb 16, 2024 at 11:06:43AM +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@opensynergy.com>
> Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com>
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1583619 ("Uninitialized variables")
> Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls")
> ---
> sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> index 0c6ac74aca1e..7aa79c05b464 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_msg_unref;
> }
>
> sg_init_one(&sg, tlv, size);
> @@ -281,14 +281,25 @@ 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
> + goto on_msg_unref;
> + } else {
> 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);
> + goto on_msg_unref;
> }
> + kfree(tlv);
> + return rc;
>
> +on_msg_unref:
> + virtsnd_ctl_msg_unref(msg);
> kfree(tlv);
>
> return rc;
I don't really like adding code for a false-positive but ALSA
maintainers seem to like this. If yes, this seems like as good
a way as any to do it.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> --
> 2.43.2
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 3+ messages in thread
* [virtio-dev] Re: [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.
[not found] ` <87eddcikz6.wl-tiwai@suse.de>
@ 2024-02-16 13:48 ` Michael S. Tsirkin
0 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2024-02-16 13:48 UTC (permalink / raw)
To: Takashi Iwai
Cc: Aiswarya Cyriac, jasowang, perex, tiwai, linux-kernel, alsa-devel,
virtualization, virtio-dev, Anton Yakovlev, coverity-bot
On Fri, Feb 16, 2024 at 02:42:37PM +0100, Takashi Iwai wrote:
> On Fri, 16 Feb 2024 12:27:48 +0100,
> Michael S. Tsirkin wrote:
> >
> > On Fri, Feb 16, 2024 at 11:06:43AM +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@opensynergy.com>
> > > Signed-off-by: Aiswarya Cyriac <aiswarya.cyriac@opensynergy.com>
> > > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> > > Addresses-Coverity-ID: 1583619 ("Uninitialized variables")
> > > Fixes: d6568e3de42d ("ALSA: virtio: add support for audio controls")
> >
> >
> >
> > > ---
> > > sound/virtio/virtio_kctl.c | 19 +++++++++++++++----
> > > 1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
> > > index 0c6ac74aca1e..7aa79c05b464 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_msg_unref;
> > > }
> > >
> > > sg_init_one(&sg, tlv, size);
> > > @@ -281,14 +281,25 @@ 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
> > > + goto on_msg_unref;
> > > + } else {
> > > 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);
> > > + goto on_msg_unref;
> > > }
> > > + kfree(tlv);
> > > + return rc;
> > >
> > > +on_msg_unref:
> > > + virtsnd_ctl_msg_unref(msg);
> > > kfree(tlv);
> > >
> > > return rc;
> >
> > I don't really like adding code for a false-positive but ALSA
> > maintainers seem to like this. If yes, this seems like as good
> > a way as any to do it.
>
> Err, no, you misunderstood the situation.
>
> I took the v1 patch quickly because:
> - It was with Anton's SOB, who is another maintainer of the driver
> - I assumed you lost interest in this driver since you haven't reacted
> to the previous patches for long time
> - The change there was small and simple enough
>
> Now, it grows unnecessarily large, and yet you complained. Why should
> I take it, then?
>
> This is a subtle cosmetic issue that isn't worth for wasting too much
> time and energy. If we want to shut up the compile warning, and this
> is a case where it can't happen, just put the "default:" to the
> existing case. If you want to be user-friendly, put some comment.
> That's all. It'll be a one-liner.
>
> OTOH, if we do care and want to catch any potential logical mistake,
> you can put WARN(). But, this doesn't have to go out as an error.
> Simply putting WARN() for the default and going through would work,
> too.
>
> Or we can keep this lengthy changes if we want, too.
>
> So, I really don't mind which way to fix as long as it works correctly
> (and doesn't look too ugly). Please make agreement among you guys,
> and resubmit if needed.
>
>
> thanks,
>
> Takashi
OK sorry about too verbose. I mean since Anton wants it, I ack this.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
--
MST
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-16 13:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-16 10:06 [virtio-dev] [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning Aiswarya Cyriac
2024-02-16 11:27 ` [virtio-dev] " Michael S. Tsirkin
[not found] ` <87eddcikz6.wl-tiwai@suse.de>
2024-02-16 13:48 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).