[MPlayer-dev-eng] [PATCH] segfault in playtree.c

[Reinhard Tartler]

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;
 }


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.

So which one of the two patches do you prefer?

-- 
Gruesse/greetings,
Reinhard Tartler, KeyID 945348A4