[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 <***@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");

[Daniel Mack]

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

Signed-off-by: Daniel Mack <***@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;
}

[Takashi Iwai]

At Sun, 19 Oct 2014 09:11:26 +0200,

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

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

Post by Daniel Mack
---
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

[Takashi Iwai]

At Sun, 19 Oct 2014 11:38:58 +0200,

Post by Takashi Iwai
At Sun, 19 Oct 2014 09:11:26 +0200,

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

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,

[Daniel Mack]

Post by Takashi Iwai
At Sun, 19 Oct 2014 11:38:58 +0200,

Post by Takashi Iwai
At Sun, 19 Oct 2014 09:11:26 +0200,

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

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

Post by Takashi Iwai
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,

[Takashi Iwai]

At Sun, 19 Oct 2014 09:11:25 +0200,

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

Thanks, applied.


Takashi

Post by Daniel Mack
---
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