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 <[email protected]> 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 -- To UNSUBSCRIBE, email to [email protected] with a subject of "unsubscribe". Trouble? Contact [email protected]
