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

[Daniel Mack <daniel@zonque.org>]

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,
>