[MPlayer-dev-eng] Bug#591525: [PATCH] segfault in playtree.c

[Reinhard Tartler]

On Fri, Aug 06, 2010 at 11:49:59 (EDT), Reimar Döffinger wrote:

> 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.

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;


>
>> 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.

Okay, so how about this:

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
   assert(pt->entry_type == PLAY_TREE_ENTRY_NODE);
 #endif
 


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