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.