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

[Reimar Döffinger]

On Fri, Aug 06, 2010 at 01:22:12PM -0400, Reinhard Tartler wrote:
> I see. In this case, I propose this:
> 
> Index: playtreeparser.c
> ===================================================================
> --- playtreeparser.c	(revision 31931)
> +++ playtreeparser.c	(working copy)
> @@ -367,6 +367,9 @@
>  
>    free(entries);
>  
> +  if (list)
> +    return NULL;
> +
>    entry = play_tree_new();
>    play_tree_set_child(entry,list);
>    return entry;

Condition inverted? I also don't know if the code
above can deal with NULL return, but in principle
I guess it should be right.
On checking again, this means that the parser code
will try to parse it as a different format.
Probably correct, but definitely a side-effect.

> Index: playtree.c
> ===================================================================
> --- playtree.c	(revision 31931)
> +++ playtree.c	(working copy)
> @@ -218,8 +218,15 @@
>  play_tree_set_child(play_tree_t* pt, play_tree_t* child) {
>    play_tree_t* iter;
>  
> -#ifdef MP_DEBUG
> -  assert(pt != NULL);
> +  /* Roughly validate input data. Both, pt and child are going to be
> +   * dereferenced, hence assure they're not NULL.
> +   */
> +  if (NULL == pt || NULL == child) {
> +    mp_msg(MSGT_PLAYTREE,MSGL_ERR, "Detected malformed playlist file. Possible bug in paser?\n");
> +    return;
> +  }
> +
> +#ifdef MP_DEBUG || 1

Mostly ok, except the ifdef part of course and also I am not
sure this is only called for playlist files.
It should be something like "Internal error, attempt to
add an empty child or use empty playlist\n".
Also "NULL == pt" should be !pt etc.
Also, a spaces is missing before MSGL.