Re: [PATCH 3.13 110/149] ASoC: pcm: free path list before exiting from error conditions

[Mark Brown]

On Sat, Mar 22, 2014 at 03:53:08PM +0000, Ben Hutchings wrote:
> On Thu, 2014-03-20 at 17:04 -0700, Greg Kroah-Hartman wrote:

If you spot something on a stable patch that applies upstream (as
opposed to being stable process or to do with the older kernel which are
the most common things with these mails) please add both the maintainers
and relevant lists.  -stable mail only goes to signoffs so misses both
lists and additional maintainers meaning things may not be going to the
people who should see them (in this case it's Liam who mostly looks at
DPCM for example).  It might make sense for the -stable mails to do this
in general, at least with the lists if not the maintainers.

Ideally it's helpful to also send it in a separate thread since the
volume of mail about -stable stuff is extremely high so things are
easily missed but that's a bit more fun to do, I'm really not sure
there's any general way to resolve that one sensibly.

> > dpcm_path_get() allocates dynamic memory to hold path list.
> > Corresponding dpcm_path_put() must be called to free the memory.
> > dpcm_path_put() is not called under several error conditions.
> > This leads to memory leak.

> This is broken.  dpcm_path_get() may return -ENOMEM and not initialise
> the list at all.

> If snd_soc_dapm_dai_get_connected_widgets() can fail (I don't think it
> can) then dpcm_path_get() should be responsible for freeing the list
> before returning.

It can't fail without memory corruption or internal bugs and would only
fail with a crash.

> [...]
> > --- a/sound/soc/soc-pcm.c
> > +++ b/sound/soc/soc-pcm.c
> [...]
> > @@ -1979,6 +1981,7 @@ static int dpcm_fe_dai_open(struct snd_p
> >  	fe->dpcm[stream].runtime = fe_substream->runtime;
> >  
> >  	if (dpcm_path_get(fe, stream, &list) <= 0) {
> > +		dpcm_path_put(&list);

> This is the one place where a memory leak seems to be possible, but the
> < 0 and == 0 cases need to be distinguished.

> Greg, please drop this until it is fixed properly upstream.

It's actually not going to cause a leak there at all since we have an
unconditional run through the rest of the function to a double free with
references to the list that gets freed, AFAICT there wasn't a leak to
start off with.  I think what the function needs to do here is bomb out
early on -ENOMEM and otherwise trundle on.

Anyway, I'll revert this upstream.  Thanks for noticing this.

Attachment: signature.asc
Description: Digital signature