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