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 <a...@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
_______________________________________________
pkg-multimedia-maintainers mailing list
pkg-multimedia-maintainers@lists.alioth.debian.org
http://lists.alioth.debian.org/mailman/listinfo/pkg-multimedia-maintainers
