From patchwork Sun Sep 3 17:09:41 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 9936449 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 14AC9601EB for ; Sun, 3 Sep 2017 17:09:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F240228684 for ; Sun, 3 Sep 2017 17:09:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E6DD9286B4; Sun, 3 Sep 2017 17:09:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=unavailable version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F135128684 for ; Sun, 3 Sep 2017 17:09:50 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 4BC96266DF0; Sun, 3 Sep 2017 19:09:48 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id 2E737266E0C; Sun, 3 Sep 2017 19:09:47 +0200 (CEST) Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id C55E7266D9D for ; Sun, 3 Sep 2017 19:09:41 +0200 (CEST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 56B2EAB9B; Sun, 3 Sep 2017 17:09:41 +0000 (UTC) Date: Sun, 03 Sep 2017 19:09:41 +0200 Message-ID: From: Takashi Iwai To: Wang YanQing In-Reply-To: <20170903161053.GA27449@udknight> References: <20170903152731.GA27302@udknight> <20170903161053.GA27449@udknight> 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/25.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [alsa-devel] [PATCH] ALSA: hda: Fix resource leak issue in snd_hda_codec_build_controls and snd_hda_codec_parse_pcms X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP On Sun, 03 Sep 2017 18:10:53 +0200, Wang YanQing wrote: > > On Sun, Sep 03, 2017 at 11:27:31PM +0800, Wang YanQing wrote: > > When patch_ops.init, patch_ops.build_pcms and patch_ops.build_controls > > return failure, we need to free resource with patch_ops.free, or we will > > get resource leak. > > > > Signed-off-by: Wang YanQing > > --- > > sound/pci/hda/hda_codec.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > > index df6b57e..4e3e613 100644 > > --- a/sound/pci/hda/hda_codec.c > > +++ b/sound/pci/hda/hda_codec.c > > @@ -2973,8 +2973,11 @@ int snd_hda_codec_build_controls(struct hda_codec *codec) > > err = codec->patch_ops.init(codec); > > if (!err && codec->patch_ops.build_controls) > > err = codec->patch_ops.build_controls(codec); > > - if (err < 0) > > + if (err < 0) { > > + if (codec->patch_ops.free) > > + codec->patch_ops.free(codec); > > return err; > > + } > > > > /* we create chmaps here instead of build_pcms */ > > err = add_std_chmaps(codec); > > @@ -3170,6 +3173,8 @@ int snd_hda_codec_parse_pcms(struct hda_codec *codec) > > if (err < 0) { > > codec_err(codec, "cannot build PCMs for #%d (error %d)\n", > > codec->core.addr, err); > > + if (codec->patch_ops.free) > > + codec->patch_ops.free(codec); > > return err; > > } > > > > -- > > 1.8.5.6.2.g3d8a54e.dirty > > I don't know whether this new patch is ok for you, but I think that > we could allocate resources in patch_ops.init, patch_ops.build_pcms > and patch_ops.build_controls separately, so I think it isn't flexible > and elegant to free all resources in all the error path cases in them > separately, so maybe it is better to use patch_ops.free as the unique > point to release all resource. In that case, it'll be easier to do that in hda_bind.c as your first patch, but skip the free for patch() call; i.e. something like below. The point is that the codec probe function does already care about the free at its error path, while others don't do generically. Or, for safety, we may put an internal flag to indicate that the codec free got already called, and check it at before calling patch_ops.free, too. thanks, Takashi diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c index 6efadbfb3fe3..d361bb77ca00 100644 --- a/sound/pci/hda/hda_bind.c +++ b/sound/pci/hda/hda_bind.c @@ -100,7 +100,7 @@ static int hda_codec_driver_probe(struct device *dev) if (patch) { err = patch(codec); if (err < 0) - goto error_module; + goto error_module_put; } err = snd_hda_codec_build_pcms(codec); @@ -120,6 +120,9 @@ static int hda_codec_driver_probe(struct device *dev) return 0; error_module: + if (codec->patch_ops.free) + codec->patch_ops.free(codec); + error_module_put: module_put(owner); error: