Bug#591525: [PATCH] segfault in playtree.c

[Reimar Döffinger]

On Thu, Aug 05, 2010 at 08:48:59PM -0400, Reinhard Tartler wrote:
> On Thu, Aug 05, 2010 at 18:00:11 (EDT), Reimar Döffinger wrote:
> 
> > On Thu, Aug 05, 2010 at 12:39:52AM -0400, Reinhard Tartler wrote:
> >> Hi Folks,
> >> 
> >> This is a patch from Adrian Knoth <adi at drcomp.erfurt.thur.de> to fix a
> >> segfault on empty playlists.
> >> 
> >> This is Debian Bug: http://bugs.debian.org/591525
> >> 
> >> Index: playtree.c
> >> ===================================================================
> >> --- playtree.c	(revision 31912)
> >> +++ playtree.c	(working copy)
> >> @@ -223,6 +223,13 @@
> >>    assert(pt->entry_type == PLAY_TREE_ENTRY_NODE);
> >>  #endif
> >>  
> >> +  /* Roughly validate input data. Both, pt and child are going to be
> >> +   * dereferenced, hence assure they're not NULL.
> >> +   */
> >> +  if (NULL == pt || NULL == child) {
> >> +      return;
> >> +  }
> >> +
> >>    //DEBUG_FF: Where are the children freed?
> >>    // Attention in using this function!
> >>    for(iter = pt->child ; iter != NULL ; iter = iter->next)
> >
> > Think I replied to the wrong place first: I think this is the wrong
> > place, at least it must be before the asserts.
> 
> I've analyzed the stacktrace more, and I think there is a bug in
> parse_pls() in playtreeparser.c. In case there is no (valid) entry, the
> for loop in line 344 is executed 0 times. This leads to leaving the
> variable 'list' to '0', which in turn causes the crash. For this reason,
> I'm proposing this patch:
> 
> Index: playtreeparser.c
> ===================================================================
> --- playtreeparser.c	(revision 31930)
> +++ playtreeparser.c	(working copy)
> @@ -368,6 +368,7 @@
>    free(entries);
>  
>    entry = play_tree_new();
> +  if (list)
>    play_tree_set_child(entry,list);
>    return entry;
>  }

I don't know how the code is supposed to handle it, but I think that
and empty playlist should be considered invalid and the return value
should indicate this.
E.g. by returning NULL instead of an empty new playtree.

> However, I still think Adrians Patch isn't bad and in this case would
> have had the exact same behavior. If perhaps other parsers have a
> similar problems, Adi's patch would be more safe.

I am not against applying an improved version (with the checks before
the asserts) as an additional safety measure, however it should
also print an error message, IMO failing at that point is an
indication the playlist parser code had a bug and failed to detect
a malformed playlist file.