Messages in this thread | | | Date | Thu, 31 Mar 2016 16:20:20 +0200 | From | Takashi Iwai <> | Subject | Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream() |
| |
On Thu, 31 Mar 2016 16:03:55 +0200, Vladis Dronov wrote: > > Hello, Takashi, all, > > > No, it has nothing to do with the double-free bug itself. Such an > > optimization shouldn't be put in a fix patch > > This piece of code move alone fixes the double-free bug in > create_fixed_stream_quirk(), so I believe it is related.
The merits you pointed are irrelevant from the double-free bug.
> Besides, a lot of stuff > is created and initialized in snd_usb_add_audio_stream() and while I do not see > another use-after-free bug, it could be there. By moving this code we avoid > these potential bugs we have not hit yet.
It's a different issue. The only judgment now is which one is clearer to understand. The code efficiency isn't a question for this bug, since the error condition is very rare, and it's no hot path, after all.
> But anyway. If you still believe this code should not be moved, please, confirm, > I'll suggest the next patch version without it.
Right, I don't want to move it.
> > Vladis, if you take someone's patch as the base, you have to carry the > > original authorship somewhere... > > Yes, I was thinking about it, I was just not sure how should I do it. Will the > following form be fine? Or somehow else? > > Based on a patch by Takashi Iwai" <tiwai@suse.de>
Yes, usually such a line should be enough.
> > > + * if not, create a new pcm stream. the caller must remove fp from > > > + * the substream fmt_list in the error path to avoid double-free. > > > > This comment isn't true. The caller may leave it as is, too, like my > > first version. It just depends on the code. > > Yes. Is the following rewrite acceptable for the next patch version? > > * if not, create a new pcm stream. Note, fp is added to the substream fmt_list > * and will be freed on the chip instance release. Do not free fp or do remove > * it from the substream fmt_list to avoid double-free.
Yes, that looks more correct.
thanks,
Takashi
| |