Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757343AbcCaOU0 (ORCPT ); Thu, 31 Mar 2016 10:20:26 -0400 Received: from mx2.suse.de ([195.135.220.15]:45396 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756543AbcCaOUW (ORCPT ); Thu, 31 Mar 2016 10:20:22 -0400 Date: Thu, 31 Mar 2016 16:20:20 +0200 Message-ID: From: Takashi Iwai To: Vladis Dronov Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-sound@vger.kernel.org Subject: Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream() In-Reply-To: <805340847.43676741.1459433035733.JavaMail.zimbra@redhat.com> References: <1459364602-28997-1-git-send-email-vdronov@redhat.com> <1459364602-28997-2-git-send-email-vdronov@redhat.com> <882494379.43642280.1459427790768.JavaMail.zimbra@redhat.com> <805340847.43676741.1459433035733.JavaMail.zimbra@redhat.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1968 Lines: 58 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" 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