Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()

[Takashi Iwai <tiwai@suse.de>]

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