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@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 | 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@xxxxxxxxxx>
--
MST
