[alsa-devel] [PATCH 1/2] ALSA: snd-usb: drop unused varible assigments
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");
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; }
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
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,
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,
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");-- 2.1.0
participants (2)
-
Daniel Mack -
Takashi Iwai