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@xxxxxxx> 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@xxxxxxx> --- 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel