Re: [v4 PATCH] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.

[Takashi Iwai]

On Fri, 16 Feb 2024 14:48:10 +0100,
Michael S. Tsirkin wrote:
> 
> 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>

Alright, I applied this one.  Thanks!


Takashi