All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: hda: Fix resource leak issue in snd_hda_codec_build_controls and snd_hda_codec_parse_pcms
@ 2017-09-03 15:27 Wang YanQing
  2017-09-03 16:10 ` Wang YanQing
  0 siblings, 1 reply; 3+ messages in thread
From: Wang YanQing @ 2017-09-03 15:27 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel, linux-kernel

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 <udknight@gmail.com>
---
 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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: hda: Fix resource leak issue in snd_hda_codec_build_controls and snd_hda_codec_parse_pcms
  2017-09-03 15:27 [PATCH] ALSA: hda: Fix resource leak issue in snd_hda_codec_build_controls and snd_hda_codec_parse_pcms Wang YanQing
@ 2017-09-03 16:10 ` Wang YanQing
  2017-09-03 17:09   ` Takashi Iwai
  0 siblings, 1 reply; 3+ messages in thread
From: Wang YanQing @ 2017-09-03 16:10 UTC (permalink / raw)
  To: tiwai, alsa-devel, linux-kernel

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 <udknight@gmail.com>
> ---
>  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.

Thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: hda: Fix resource leak issue in snd_hda_codec_build_controls and snd_hda_codec_parse_pcms
  2017-09-03 16:10 ` Wang YanQing
@ 2017-09-03 17:09   ` Takashi Iwai
  0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2017-09-03 17:09 UTC (permalink / raw)
  To: Wang YanQing; +Cc: alsa-devel, linux-kernel

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 <udknight@gmail.com>
> > ---
> >  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:

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-03 17:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-03 15:27 [PATCH] ALSA: hda: Fix resource leak issue in snd_hda_codec_build_controls and snd_hda_codec_parse_pcms Wang YanQing
2017-09-03 16:10 ` Wang YanQing
2017-09-03 17:09   ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.