[PATCH 1/2] ALSA: snd-usb: drop unused varible assigments

[PATCH 1/2] ALSA: snd-usb: drop unused varible assigments

[Daniel Mack]

Don't assign 'len' in cases where we don't make use of the returned value.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 sound/usb/mixer.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 2e4a9db..63a8adb 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1290,9 +1290,8 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
 						    kctl->id.name,
 						    sizeof(kctl->id.name), 1);
 			if (!len)
-				len = snprintf(kctl->id.name,
-					       sizeof(kctl->id.name),
-					       "Feature %d", unitid);
+				snprintf(kctl->id.name, sizeof(kctl->id.name),
+					 "Feature %d", unitid);
 		}
 
 		if (!mapped_name)
@@ -1305,9 +1304,9 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
 		 */
 		if (!mapped_name && !(state->oterm.type >> 16)) {
 			if ((state->oterm.type & 0xff00) == 0x0100)
-				len = append_ctl_name(kctl, " Capture");
+				append_ctl_name(kctl, " Capture");
 			else
-				len = append_ctl_name(kctl, " Playback");
+				append_ctl_name(kctl, " Playback");
 		}
 		append_ctl_name(kctl, control == UAC_FU_MUTE ?
 				" Switch" : " Volume");
-- 
2.1.0

[PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c

[Daniel Mack]

Out of principles, use strncpy() in favor of strcpy().
That is, however, an insignificant detail here.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 sound/usb/mixer_quirks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index f119a41..f406305 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -446,8 +446,9 @@ static int snd_emu0204_ch_switch_info(struct snd_kcontrol *kcontrol,
 	uinfo->value.enumerated.items = 2;
 	if (uinfo->value.enumerated.item > 1)
 		uinfo->value.enumerated.item = 1;
-	strcpy(uinfo->value.enumerated.name,
-		texts[uinfo->value.enumerated.item]);
+	strncpy(uinfo->value.enumerated.name,
+		texts[uinfo->value.enumerated.item],
+		sizeof(uinfo->value.enumerated.name) - 1);
 
 	return 0;
 }
-- 
2.1.0

Re: [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments

[Takashi Iwai]

At Sun, 19 Oct 2014 09:11:25 +0200,
Daniel Mack wrote:
> 
> Don't assign 'len' in cases where we don't make use of the returned value.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>

Thanks, applied.


Takashi

> ---
>  sound/usb/mixer.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 2e4a9db..63a8adb 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -1290,9 +1290,8 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
>  						    kctl->id.name,
>  						    sizeof(kctl->id.name), 1);
>  			if (!len)
> -				len = snprintf(kctl->id.name,
> -					       sizeof(kctl->id.name),
> -					       "Feature %d", unitid);
> +				snprintf(kctl->id.name, sizeof(kctl->id.name),
> +					 "Feature %d", unitid);
>  		}
>  
>  		if (!mapped_name)
> @@ -1305,9 +1304,9 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc,
>  		 */
>  		if (!mapped_name && !(state->oterm.type >> 16)) {
>  			if ((state->oterm.type & 0xff00) == 0x0100)
> -				len = append_ctl_name(kctl, " Capture");
> +				append_ctl_name(kctl, " Capture");
>  			else
> -				len = append_ctl_name(kctl, " Playback");
> +				append_ctl_name(kctl, " Playback");
>  		}
>  		append_ctl_name(kctl, control == UAC_FU_MUTE ?
>  				" Switch" : " Volume");
> -- 
> 2.1.0
> 

Re: [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c

[Takashi Iwai]

At Sun, 19 Oct 2014 09:11:26 +0200,
Daniel Mack wrote:
> 
> Out of principles, use strncpy() in favor of strcpy().
> That is, however, an insignificant detail here.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>

Well, blindly doing this isn't optimal, IMO.
First off, strlcpy() is a better one.  And, in the code you patched,
we already know all strings to be passed.  That is, if anything is
over the buffer size, it's a clear bug.  This can be caught by static
analyzers, or put some debug codes (either for build time or compile
time) instead of silently trimming the string.


thanks,

Takashi

> ---
>  sound/usb/mixer_quirks.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index f119a41..f406305 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -446,8 +446,9 @@ static int snd_emu0204_ch_switch_info(struct snd_kcontrol *kcontrol,
>  	uinfo->value.enumerated.items = 2;
>  	if (uinfo->value.enumerated.item > 1)
>  		uinfo->value.enumerated.item = 1;
> -	strcpy(uinfo->value.enumerated.name,
> -		texts[uinfo->value.enumerated.item]);
> +	strncpy(uinfo->value.enumerated.name,
> +		texts[uinfo->value.enumerated.item],
> +		sizeof(uinfo->value.enumerated.name) - 1);
>  
>  	return 0;
>  }
> -- 
> 2.1.0
> 

Re: [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c

[Takashi Iwai]

At Sun, 19 Oct 2014 11:38:58 +0200,
Takashi Iwai wrote:
> 
> At Sun, 19 Oct 2014 09:11:26 +0200,
> Daniel Mack wrote:
> > 
> > Out of principles, use strncpy() in favor of strcpy().
> > That is, however, an insignificant detail here.
> > 
> > Signed-off-by: Daniel Mack <daniel@zonque.org>
> 
> Well, blindly doing this isn't optimal, IMO.
> First off, strlcpy() is a better one.  And, in the code you patched,
> we already know all strings to be passed.  That is, if anything is
> over the buffer size, it's a clear bug.  This can be caught by static
> analyzers, or put some debug codes (either for build time or compile
> time) instead of silently trimming the string.

BTW, there is already a nice helper function, snd_ctl_enum_info(), for
the safe enum info setup.  Then the patch would become even more
reducing, something like below.  We can cover many other places in
similar ways.

A further step would be to add a kernel warning when the given string
is too long as an enum item string.  Then we can catch the buggy
driver, too.  I'll cook up the patch.


Takashi

---
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index f119a41ed9a9..dd4d5bdea423 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -441,15 +441,7 @@ static int snd_emu0204_ch_switch_info(struct snd_kcontrol *kcontrol,
 				       "3/4"
 	};
 
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
-	uinfo->count = 1;
-	uinfo->value.enumerated.items = 2;
-	if (uinfo->value.enumerated.item > 1)
-		uinfo->value.enumerated.item = 1;
-	strcpy(uinfo->value.enumerated.name,
-		texts[uinfo->value.enumerated.item]);
-
-	return 0;
+	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
 }
 
 static int snd_emu0204_ch_switch_get(struct snd_kcontrol *kcontrol,
@@ -745,15 +737,7 @@ static int snd_ftu_eff_switch_info(struct snd_kcontrol *kcontrol,
 				       "Echo"
 	};
 
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
-	uinfo->count = 1;
-	uinfo->value.enumerated.items = 8;
-	if (uinfo->value.enumerated.item > 7)
-		uinfo->value.enumerated.item = 7;
-	strcpy(uinfo->value.enumerated.name,
-		texts[uinfo->value.enumerated.item]);
-
-	return 0;
+	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
 }
 
 static int snd_ftu_eff_switch_get(struct snd_kcontrol *kctl,

Re: [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c

[Daniel Mack]

On 10/20/2014 04:40 PM, Takashi Iwai wrote:
> At Sun, 19 Oct 2014 11:38:58 +0200,
> Takashi Iwai wrote:
>>
>> At Sun, 19 Oct 2014 09:11:26 +0200,
>> Daniel Mack wrote:
>>>
>>> Out of principles, use strncpy() in favor of strcpy().
>>> That is, however, an insignificant detail here.
>>>
>>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>>
>> Well, blindly doing this isn't optimal, IMO.
>> First off, strlcpy() is a better one.  And, in the code you patched,
>> we already know all strings to be passed.  That is, if anything is
>> over the buffer size, it's a clear bug.  This can be caught by static
>> analyzers, or put some debug codes (either for build time or compile
>> time) instead of silently trimming the string.
> 
> BTW, there is already a nice helper function, snd_ctl_enum_info(), for
> the safe enum info setup.  Then the patch would become even more
> reducing, something like below.  We can cover many other places in
> similar ways.
> 
> A further step would be to add a kernel warning when the given string
> is too long as an enum item string.  Then we can catch the buggy
> driver, too.  I'll cook up the patch.

Sounds good to me. And yes, we know which values we write to that buffer
and that they can't possibly explode it. I was just thinking of the next
reader of the code who might be quicker in the review with some obvious
guards in place.


Thanks,
Daniel


> 
> 
> Takashi
> 
> ---
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index f119a41ed9a9..dd4d5bdea423 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -441,15 +441,7 @@ static int snd_emu0204_ch_switch_info(struct snd_kcontrol *kcontrol,
>  				       "3/4"
>  	};
>  
> -	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> -	uinfo->count = 1;
> -	uinfo->value.enumerated.items = 2;
> -	if (uinfo->value.enumerated.item > 1)
> -		uinfo->value.enumerated.item = 1;
> -	strcpy(uinfo->value.enumerated.name,
> -		texts[uinfo->value.enumerated.item]);
> -
> -	return 0;
> +	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
>  }
>  
>  static int snd_emu0204_ch_switch_get(struct snd_kcontrol *kcontrol,
> @@ -745,15 +737,7 @@ static int snd_ftu_eff_switch_info(struct snd_kcontrol *kcontrol,
>  				       "Echo"
>  	};
>  
> -	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> -	uinfo->count = 1;
> -	uinfo->value.enumerated.items = 8;
> -	if (uinfo->value.enumerated.item > 7)
> -		uinfo->value.enumerated.item = 7;
> -	strcpy(uinfo->value.enumerated.name,
> -		texts[uinfo->value.enumerated.item]);
> -
> -	return 0;
> +	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
>  }
>  
>  static int snd_ftu_eff_switch_get(struct snd_kcontrol *kctl,
>