[alsa-devel] [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path
Takashi Iwai
tiwai at suse.de
Wed Feb 22 08:46:59 CET 2017
On Wed, 22 Feb 2017 08:44:13 +0100,
Takashi Iwai wrote:
>
> On Wed, 22 Feb 2017 02:42:39 +0100,
> Takashi Sakamoto wrote:
> >
> > On Feb 22 2017 07:59, Takashi Sakamoto wrote:
> > > On Feb 22 2017 07:45, Takashi Sakamoto wrote:
> > >> On Fe 21 2017 11:32, Takashi Sakamoto wrote:
> > >>> But it's wrong. The allocated memory objects are used for private data
> > >>> for each control element set. I didn't care of it...
> > >>>
> > >>> Here, what I should fix is just for error path. I'll post revised
> > >>> version of this patch this night.
> > >>
> > >> I realize that this is not concern because the code includes assignment
> > >> deallocate function to each control element set. Thus, in error path,
> > >> the allocated private data is going to free in below callgrach.
> > >>
> > >> (sound/usb/mixer_us16x08.c)
> > >> add_new_ctl(elem)
> > >> kctl->private_free = freeer;
> > >> (sound/usb/mixer.c)
> > >> ->snd_usb_mixer_add_control(kctl)
> > >> (sound/core/control.c)
> > >> ->snd_ctl_add(kctl)
> > >> ->snd_ctl_free_one(kctl)
> > >> (in error path)
> > >> ->kcontrol->private_free(kctl);
> > >> (sound/usb/mixer_us16x08.c)
> > >> = freeer()
> > >
> > > Oops. On the other hand, the private data for unregistered control
> > > element set is not deallocated automatically in error path of the other
> > > control element set. It should be fixed...
> >
> > Mmm. There might be an issue of double free corruption, I think.
> >
> > >> (sound/core/control.c)
> > >> ->snd_ctl_add(kctl)
> > >> (in error path)
> > >> ->snd_ctl_free_one(kctl)
> > >> ->kcontrol->private_free(kctl);
> > >> (sound/usb/mixer_us16x08.c)
> > >> = freeer()
> >
> > This 'freeer()' is set in beginning of the graph.
> >
> > >> (sound/usb/mixer_us16x08.c)
> > >> add_new_ctl(elem)
> > >> kctl->private_free = freeer;
> >
> > The memory objects are not allocated for each of the control element
> > set. On the other hand, the calls of the 'freeer()' are corresponding
> > to each control element set. When the ratio between
> > .private_data/.private_free and control element set is 1:N,
> > .private_free can cause double free corruption at freeing control
> > character device in kernel side.
> >
> > In current code, there're two cases; NULL or
> > 'snd_usb_mixer_elem_free()' are assigned as the 'freeer()'. But this
> > is not enough. Actually, the allocated memory object for the
> > 'comp_store' variable is going to be deallocated by two control
> > element sets; for 'channel_controls[0]' and 'channel_controls[0]' the
> > allocated memory object for the 'eq_sotre' variable is going to be
> > deallocated for several control element sets; the 'eq_controls[0, 1,
> > 3, 6, 9]'.
> >
> > As long as I understand, current code has the above issue and can
> > bring kernel corruption. If I missing something, please inform it to
> > me.
>
> Yeah, there are quite a few memory leaks and double frees.
> The patch below is a bit of drastic surgery.
... and yet more cleanup patch on top of it.
Takashi
-- 8< --
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] ALSA: usb-audio: Tidy up mixer_us16x08.c
A few more cleanups and improvements that have been overlooked:
- Use ARRAY_SIZE() macro appropriately
- Code shuffling for minor optimization
- Omit superfluous variable initializations
- Get rid of superfluous NULL checks
- Add const to snd_us16x08_control_params definitions
No functional changes.
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
sound/usb/mixer_us16x08.c | 132 ++++++++++++++++++----------------------------
1 file changed, 50 insertions(+), 82 deletions(-)
diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index f7289541fbce..dc48eedea92e 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -176,15 +176,9 @@ static int snd_us16x08_recv_urb(struct snd_usb_audio *chip,
*/
static int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size)
{
- int err = 0;
-
- if (chip) {
- err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
+ return snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0),
SND_US16X08_URB_REQUEST, SND_US16X08_URB_REQUESTTYPE,
0, 0, buf, size);
- }
-
- return err;
}
static int snd_us16x08_route_info(struct snd_kcontrol *kcontrol,
@@ -212,10 +206,7 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
struct snd_usb_audio *chip = elem->head.mixer->chip;
int index = ucontrol->id.index;
char buf[sizeof(route_msg)];
- int val, val_org, err = 0;
-
- /* prepare the message buffer from template */
- memcpy(buf, route_msg, sizeof(route_msg));
+ int val, val_org, err;
/* get the new value (no bias for routes) */
val = ucontrol->value.enumerated.item[0];
@@ -224,6 +215,9 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol,
if (val < 0 || val > 9)
return -EINVAL;
+ /* prepare the message buffer from template */
+ memcpy(buf, route_msg, sizeof(route_msg));
+
if (val < 2) {
/* input comes from a master channel */
val_org = val;
@@ -279,12 +273,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol,
struct usb_mixer_elem_info *elem = kcontrol->private_data;
struct snd_usb_audio *chip = elem->head.mixer->chip;
char buf[sizeof(mix_msg_out)];
- int val, err = 0;
+ int val, err;
int index = ucontrol->id.index;
- /* prepare the message buffer from template */
- memcpy(buf, mix_msg_out, sizeof(mix_msg_out));
-
/* new control value incl. bias*/
val = ucontrol->value.integer.value[0];
@@ -293,6 +284,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol,
|| val > SND_US16X08_KCMAX(kcontrol))
return -EINVAL;
+ /* prepare the message buffer from template */
+ memcpy(buf, mix_msg_out, sizeof(mix_msg_out));
+
buf[8] = val - SND_US16X08_KCBIAS(kcontrol);
buf[6] = elem->head.id;
@@ -392,9 +386,6 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol,
int val, err;
int index = ucontrol->id.index;
- /* prepare URB message from template */
- memcpy(buf, mix_msg_in, sizeof(mix_msg_in));
-
val = ucontrol->value.integer.value[0];
/* sanity check */
@@ -402,6 +393,9 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol,
|| val > SND_US16X08_KCMAX(kcontrol))
return -EINVAL;
+ /* prepare URB message from template */
+ memcpy(buf, mix_msg_in, sizeof(mix_msg_in));
+
/* add the bias to the new value */
buf[8] = val - SND_US16X08_KCBIAS(kcontrol);
buf[6] = elem->head.id;
@@ -434,8 +428,7 @@ static int snd_us16x08_comp_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct usb_mixer_elem_info *elem = kcontrol->private_data;
- struct snd_us16x08_comp_store *store =
- ((struct snd_us16x08_comp_store *) elem->private_data);
+ struct snd_us16x08_comp_store *store = elem->private_data;
int index = ucontrol->id.index;
int val_idx = COMP_STORE_IDX(elem->head.id);
@@ -449,18 +442,11 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
{
struct usb_mixer_elem_info *elem = kcontrol->private_data;
struct snd_usb_audio *chip = elem->head.mixer->chip;
- struct snd_us16x08_comp_store *store =
- ((struct snd_us16x08_comp_store *) elem->private_data);
+ struct snd_us16x08_comp_store *store = elem->private_data;
int index = ucontrol->id.index;
char buf[sizeof(comp_msg)];
int val_idx, val;
- int err = 0;
-
- /* prepare compressor URB message from template */
- memcpy(buf, comp_msg, sizeof(comp_msg));
-
- /* new control value incl. bias*/
- val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE;
+ int err;
val = ucontrol->value.integer.value[0];
@@ -469,8 +455,14 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
|| val > SND_US16X08_KCMAX(kcontrol))
return -EINVAL;
+ /* new control value incl. bias*/
+ val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE;
+
store->val[val_idx][index] = ucontrol->value.integer.value[0];
+ /* prepare compressor URB message from template */
+ memcpy(buf, comp_msg, sizeof(comp_msg));
+
/* place comp values in message buffer watch bias! */
buf[8] = store->val[
COMP_STORE_IDX(SND_US16X08_ID_COMP_THRESHOLD)][index]
@@ -502,10 +494,9 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol,
static int snd_us16x08_eqswitch_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
- int val = 0;
+ int val;
struct usb_mixer_elem_info *elem = kcontrol->private_data;
- struct snd_us16x08_eq_store *store =
- ((struct snd_us16x08_eq_store *) elem->private_data);
+ struct snd_us16x08_eq_store *store = elem->private_data;
int index = ucontrol->id.index;
/* get low switch from cache is enough, cause all bands are together */
@@ -521,10 +512,8 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
{
struct usb_mixer_elem_info *elem = kcontrol->private_data;
struct snd_usb_audio *chip = elem->head.mixer->chip;
- struct snd_us16x08_eq_store *store =
- ((struct snd_us16x08_eq_store *) elem->private_data);
+ struct snd_us16x08_eq_store *store = elem->private_data;
int index = ucontrol->id.index;
-
char buf[sizeof(eqs_msq)];
int val, err = 0;
int b_idx;
@@ -564,10 +553,9 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol,
static int snd_us16x08_eq_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
- int val = 0;
+ int val;
struct usb_mixer_elem_info *elem = kcontrol->private_data;
- struct snd_us16x08_eq_store *store =
- ((struct snd_us16x08_eq_store *) elem->private_data);
+ struct snd_us16x08_eq_store *store = elem->private_data;
int index = ucontrol->id.index;
int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1;
int p_idx = EQ_STORE_PARAM_IDX(elem->head.id);
@@ -584,17 +572,13 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol,
{
struct usb_mixer_elem_info *elem = kcontrol->private_data;
struct snd_usb_audio *chip = elem->head.mixer->chip;
- struct snd_us16x08_eq_store *store =
- ((struct snd_us16x08_eq_store *) elem->private_data);
+ struct snd_us16x08_eq_store *store = elem->private_data;
int index = ucontrol->id.index;
char buf[sizeof(eqs_msq)];
- int val, err = 0;
+ int val, err;
int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1;
int p_idx = EQ_STORE_PARAM_IDX(elem->head.id);
- /* copy URB buffer from EQ template */
- memcpy(buf, eqs_msq, sizeof(eqs_msq));
-
val = ucontrol->value.integer.value[0];
/* sanity check */
@@ -602,6 +586,9 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol,
|| val > SND_US16X08_KCMAX(kcontrol))
return -EINVAL;
+ /* copy URB buffer from EQ template */
+ memcpy(buf, eqs_msq, sizeof(eqs_msq));
+
store->val[b_idx][p_idx][index] = val;
buf[20] = store->val[b_idx][3][index];
buf[17] = store->val[b_idx][2][index];
@@ -713,12 +700,6 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol,
u8 meter_urb[64];
char tmp[sizeof(mix_init_msg2)] = {0};
- if (elem) {
- store = (struct snd_us16x08_meter_store *) elem->private_data;
- chip = elem->head.mixer->chip;
- } else
- return 0;
-
switch (kcontrol->private_value) {
case 0:
snd_us16x08_send_urb(chip, (char *)mix_init_msg1,
@@ -983,11 +964,11 @@ static struct snd_kcontrol_new snd_us16x08_meter_ctl = {
/* setup compressor store and assign default value */
static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
{
- int i = 0;
- struct snd_us16x08_comp_store *tmp =
- kmalloc(sizeof(struct snd_us16x08_comp_store), GFP_KERNEL);
+ int i;
+ struct snd_us16x08_comp_store *tmp;
- if (tmp == NULL)
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
return NULL;
for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
@@ -1006,10 +987,10 @@ static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void)
static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
{
int i, b_idx;
- struct snd_us16x08_eq_store *tmp =
- kmalloc(sizeof(struct snd_us16x08_eq_store), GFP_KERNEL);
+ struct snd_us16x08_eq_store *tmp;
- if (tmp == NULL)
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
return NULL;
for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) {
@@ -1042,15 +1023,14 @@ static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void)
static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)
{
- struct snd_us16x08_meter_store *tmp =
- kzalloc(sizeof(struct snd_us16x08_meter_store), GFP_KERNEL);
+ struct snd_us16x08_meter_store *tmp;
+ tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
if (!tmp)
return NULL;
tmp->comp_index = 1;
tmp->comp_active_index = 0;
return tmp;
-
}
/* release elem->private_free as well; called only once for each *_store */
@@ -1067,7 +1047,7 @@ static void elem_private_free(struct snd_kcontrol *kctl)
static int add_new_ctl(struct usb_mixer_interface *mixer,
const struct snd_kcontrol_new *ncontrol,
int index, int val_type, int channels,
- const char *name, const void *opt,
+ const char *name, void *opt,
bool do_private_free,
struct usb_mixer_elem_info **elem_ret)
{
@@ -1088,7 +1068,7 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
elem->head.id = index;
elem->val_type = val_type;
elem->channels = channels;
- elem->private_data = (void *) opt;
+ elem->private_data = opt;
kctl = snd_ctl_new1(ncontrol, elem);
if (!kctl) {
@@ -1113,10 +1093,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
return 0;
}
-static struct snd_us16x08_control_params control_params;
-
/* table of EQ controls */
-static struct snd_us16x08_control_params eq_controls[] = {
+static const struct snd_us16x08_control_params eq_controls[] = {
{ /* EQ switch */
.kcontrol_new = &snd_us16x08_eq_switch_ctl,
.control_id = SND_US16X08_ID_EQENABLE,
@@ -1197,7 +1175,7 @@ static struct snd_us16x08_control_params eq_controls[] = {
};
/* table of compressor controls */
-static struct snd_us16x08_control_params comp_controls[] = {
+static const struct snd_us16x08_control_params comp_controls[] = {
{ /* Comp enable */
.kcontrol_new = &snd_us16x08_compswitch_ctl,
.control_id = SND_US16X08_ID_COMP_SWITCH,
@@ -1243,7 +1221,7 @@ static struct snd_us16x08_control_params comp_controls[] = {
};
/* table of channel controls */
-static struct snd_us16x08_control_params channel_controls[] = {
+static const struct snd_us16x08_control_params channel_controls[] = {
{ /* Phase */
.kcontrol_new = &snd_us16x08_ch_boolean_ctl,
.control_id = SND_US16X08_ID_PHASE,
@@ -1279,7 +1257,7 @@ static struct snd_us16x08_control_params channel_controls[] = {
};
/* table of master controls */
-static struct snd_us16x08_control_params master_controls[] = {
+static const struct snd_us16x08_control_params master_controls[] = {
{ /* Master */
.kcontrol_new = &snd_us16x08_master_ctl,
.control_id = SND_US16X08_ID_FADER,
@@ -1347,10 +1325,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
return -ENOMEM;
/* add master controls */
- for (i = 0;
- i < sizeof(master_controls)
- / sizeof(control_params);
- i++) {
+ for (i = 0; i < ARRAY_SIZE(master_controls); i++) {
err = add_new_ctl(mixer,
master_controls[i].kcontrol_new,
@@ -1368,10 +1343,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
}
/* add channel controls */
- for (i = 0;
- i < sizeof(channel_controls)
- / sizeof(control_params);
- i++) {
+ for (i = 0; i < ARRAY_SIZE(channel_controls); i++) {
err = add_new_ctl(mixer,
channel_controls[i].kcontrol_new,
@@ -1396,8 +1368,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
return -ENOMEM;
/* add EQ controls */
- for (i = 0; i < sizeof(eq_controls) /
- sizeof(control_params); i++) {
+ for (i = 0; i < ARRAY_SIZE(eq_controls); i++) {
err = add_new_ctl(mixer,
eq_controls[i].kcontrol_new,
@@ -1413,10 +1384,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
}
/* add compressor controls */
- for (i = 0;
- i < sizeof(comp_controls)
- / sizeof(control_params);
- i++) {
+ for (i = 0; i < ARRAY_SIZE(comp_controls); i++) {
err = add_new_ctl(mixer,
comp_controls[i].kcontrol_new,
--
2.11.1
More information about the Alsa-devel
mailing list