diff mbox series

[v3] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning.

Message ID 20240214140110.606171-1-aiswarya.cyriac@opensynergy.com
State New
Headers
Series [v3] ALSA: virtio: Fix "Coverity: virtsnd_kctl_tlv_op(): Uninitialized variables" warning. |

Commit Message

Aiswarya Cyriac Feb. 14, 2024, 2:01 p.m. UTC 
  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 | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin Feb. 14, 2024, 3:36 p.m. UTC | #1 
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@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 | 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
Aiswarya Cyriac Feb. 15, 2024, 9:12 a.m. UTC | #2 
>>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@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 | 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@opensynergy.com

www.opensynergy.com
Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Régis Adjamah

________________________________________
From: Michael S. Tsirkin <mst@redhat.com>
Sent: Wednesday, February 14, 2024 4:36 PM
To: Aiswarya Cyriac
Cc: jasowang@redhat.com; perex@perex.cz; tiwai@suse.com; linux-kernel@vger.kernel.org; alsa-devel@alsa-project.org; virtualization@lists.linux-foundation.org; virtio-dev@lists.oasis-open.org; 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@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 | 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
diff mbox series

Patch

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.
+		 */
+		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 */
+			virtsnd_ctl_msg_ref(msg);
+
 			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
+		}
+
+		break;
+	default:
+		rc = -EINVAL;
+		WARN_ON(1);
 
 		break;
 	}
 
+on_cleanup:
+	virtsnd_ctl_msg_unref(msg);
+
 	kfree(tlv);
 
 	return rc;