* [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments @ 2014-10-19 7:11 Daniel Mack 2014-10-19 7:11 ` [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c Daniel Mack 2014-10-19 9:36 ` [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments Takashi Iwai 0 siblings, 2 replies; 6+ messages in thread From: Daniel Mack @ 2014-10-19 7:11 UTC (permalink / raw) To: alsa-devel; +Cc: tiwai, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c 2014-10-19 7:11 [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments Daniel Mack @ 2014-10-19 7:11 ` Daniel Mack 2014-10-19 9:38 ` Takashi Iwai 2014-10-19 9:36 ` [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments Takashi Iwai 1 sibling, 1 reply; 6+ messages in thread From: Daniel Mack @ 2014-10-19 7:11 UTC (permalink / raw) To: alsa-devel; +Cc: tiwai, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c 2014-10-19 7:11 ` [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c Daniel Mack @ 2014-10-19 9:38 ` Takashi Iwai 2014-10-20 14:40 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2014-10-19 9:38 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c 2014-10-19 9:38 ` Takashi Iwai @ 2014-10-20 14:40 ` Takashi Iwai 2014-10-20 14:47 ` Daniel Mack 0 siblings, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2014-10-20 14:40 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel 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, ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c 2014-10-20 14:40 ` Takashi Iwai @ 2014-10-20 14:47 ` Daniel Mack 0 siblings, 0 replies; 6+ messages in thread From: Daniel Mack @ 2014-10-20 14:47 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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, > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments 2014-10-19 7:11 [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments Daniel Mack 2014-10-19 7:11 ` [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c Daniel Mack @ 2014-10-19 9:36 ` Takashi Iwai 1 sibling, 0 replies; 6+ messages in thread From: Takashi Iwai @ 2014-10-19 9:36 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-20 14:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-19 7:11 [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments Daniel Mack 2014-10-19 7:11 ` [PATCH 2/2] ALSA: snd-usb: use strncpy() in mixer_quirks.c Daniel Mack 2014-10-19 9:38 ` Takashi Iwai 2014-10-20 14:40 ` Takashi Iwai 2014-10-20 14:47 ` Daniel Mack 2014-10-19 9:36 ` [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox