From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream() Date: Thu, 31 Mar 2016 16:20:20 +0200 Message-ID: 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> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <805340847.43676741.1459433035733.JavaMail.zimbra@redhat.com> Sender: linux-kernel-owner@vger.kernel.org To: Vladis Dronov Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-sound@vger.kernel.org List-Id: alsa-devel@alsa-project.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Date: Thu, 31 Mar 2016 14:20:20 +0000 Subject: Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream() Message-Id: List-Id: 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> In-Reply-To: <805340847.43676741.1459433035733.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Vladis Dronov Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-sound@vger.kernel.org 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