• Status Closed
  • Percent Complete
  • Task Type Bugs
  • Category User Interface → Themes
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.6
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Marianne Arnold - 2010-08-31
Last edited by Jonathan Gordon - 2010-09-26

FS#11590 - Skin parser chokes on too many nested conditionals (Ondio at least)

After an update to a current build (r27868)I got a problem with my theme on the Ondio which showed as “when it actually has to start playback and go to the WPS, then the player freezes with the ‘loading’ splash”. This did not happen with other theme and so I did a bit of testing and found that the following line with nested conditionals was the culprit - the theme worked correctly before. Further testing went on with a test.wps that only contained this one line. It turned out that if I reduce the level of nesting by one, I get to the WPS but the player freezes after finishing buffering. If the nesting is reduced one more level, everything works correctly.

Here is the history of what I tried:

1. original
%s%?iG<%?iC<%iC|%iG>|%?id<%id|%?ia<. %?d(1)<%d(1)|Root>|.. %?d(2)<%d(2)|%?d(1)<%d(1)|Root»»>

2. replaced some unusual tags like %iG and %iC with more common ones but unfortunately I didn’t keep it

3. simplified one level - fails on buffering when it has to take the else path (no title tag)
%?it<%it|%?ia<. %?d(1)<%d(1)|Root>|.. %?d(2)<%d(2)|%?d(1)<%d(1)|Root»»

4. one level less and this works
%?ia<. %?d(1)<%d(1)|Root>|.. %?d(2)<%d(2)|%?d(1)<%d(1)|Root»>

Together with Lear in IRC I narrowed down when the issue started and he had some ideas what was going on (see logs at - 21:17 till 23:06). The issue started with revision 27846 (the %?if tag addition), I had to backport a fix for playback on hwcodec players which helped finding this. For testing purposes Lear provided a small “fix” about which he said that it’ll break if I use %?if though but helps finding the root of the problem - here’s the diff: .

Closed by  Jonathan Gordon
2010-09-26 06:43
Reason for closing:  Fixed
Additional comments about closing:  

in r28167

Jonathan Gordon commented on 2010-09-15 11:06

I’m very surprised that diff fixes it, but if it actually does then so should this one which doesnt break %if

   fix.diff (3.5 KiB)
Marianne Arnold commented on 2010-09-16 23:54

I just tried a build with this patch on my Ondio (based on r28095) and the issue remains with my theme which uses the original nested line. A simpler theme does not freeze the player. Thanks for looking into this though.

Jonathan Gordon commented on 2010-09-17 00:47

are you 100% certain Lear’s diff fixed the problem? I honestly dont see how it could.

Marianne Arnold commented on 2010-09-17 21:34

It fixed the problem with r27899. When applied to r28095 instead of the patch above, I get a crash with address in an undefined area, somwhere “in the wild”.

Magnus Holmgren commented on 2010-09-18 10:02

The problem is the “char temp_buf[MAX_PATH];” in the if-handling code. It increases the stack usage of get_token_value noticeably (and my diff reduces it), so the crash is probably due to a stack overflow. Changing a “switch” to an “if” (if I read your patch correctly) doesn’t change stack usage (everything is allocated on function entry).

skin_render_line has a similar problem. tempbuf is only used in one specific case too. That buffer is smaller, but skin_render_line is called recursively (not quite sure under what conditions though), making the problem worse.

If the temp buffers can’t easily be removed or shared (by having one common static buffer), one way to fix it would be to move the code that actually use them to a separate non-inlined function. Then they are only allocated when actually needed. alloca might work too (it works in the Tremor codec…). Neither completely removes the possibility of a stack overflow though…

Jonathan Gordon commented on 2010-09-18 10:09

Oh, I thought gcc allocated stack variables in the scope they are defined in. I’ll move the if handling into another function then.

skin_render_line() is indeed a problem and I actually want to make that non-recursive (with a local stack+loop) but it isnt exactly trivial to do.


Available keyboard shortcuts


Task Details

Task Editing