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

[Reinhard Tartler]

On Fri, Aug 06, 2010 at 15:01:15 (EDT), Reimar Döffinger wrote:

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

D'oh, so this should be better:

Index: playtreeparser.c
===================================================================
--- playtreeparser.c	(revision 31935)
+++ 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;


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

So how about this:

Index: playtree.c
===================================================================
--- playtree.c	(revision 31935)
+++ playtree.c	(working copy)
@@ -218,8 +218,15 @@
 play_tree_set_child(play_tree_t* pt, play_tree_t* child) {
   play_tree_t* iter;
 
+  /* Roughly validate input data. Both, pt and child are going to be
+   * dereferenced, hence assure they're not NULL.
+   */
+  if (!pt || !child) {
+    mp_msg(MSGT_PLAYTREE, MSGL_ERR, "Internal error, attempt to add an empty child or use empty playlist\n");
+    return;
+  }
+
 #ifdef MP_DEBUG
-  assert(pt != NULL);
   assert(pt->entry_type == PLAY_TREE_ENTRY_NODE);
 #endif
 


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