Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category Database
  • Assigned To No-one
  • Operating System iPod 5G
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by dalesd - 2008-11-02
Last edited by learman - 2008-11-04

FS#9522 - Initializing database gives Data abort error

http://forums.rockbox.org/index.php?topic=19248.0

When I go to Database, it says, “Database is not ready” so I push Select to initialize it. It finds roughly 1100 songs, then it stops and says,
“Data abort at 0004DA64 (0)”

I too the advice of nls and found the file causing the error. See attached. (It’s a Formula One Renault V10 playing “We Are The Champions”)

Closed by  learman
2008-11-04 20:02
Reason for closing:  Fixed
Additional comments about closing:  

Fixed in r19005. One could try to handle this particular problem more gracefully (e.g., ignoring the data length indicator if the read data length is "large enough"), but why waste code on that if the tag is incorrect? :)

Here’s the file responsible for the problem.

   SOIZ.mp3 (552.5 KiB)
nls commented on 2008-11-02 10:34

This file also crashes a sim when I try to play it.
The problem turns out to be that in apps/metadata/mp3.c:489
we send a string to iso_decode() with a length of -1 and since
iso_decode() uses while(count–){} this doesn’y go too well.
Attached is a patch that fixes iso_decode() to deal with negative
lengths and so the file now plays correctly for me, there is still
something funny with the tag though. The title is shown as “are the champions”

This tag is problematic (it’s not the first time I see problems with iTunes tags by the way)

And I mention iTunes because of the 4 letters naming, and the comment frame being named “iTunNORM”

In all text frames the p flag bit is set.

From id3v2.4 spec:

 p - Data length indicator
    This flag indicates that a data length indicator has been added to
    the frame. The data length indicator is the value one would write
    as the 'Frame length' if all of the frame format flags were
    zeroed, represented as a 32 bit synchsafe integer.
    0      There is no Data Length Indicator.
    1      A data length Indicator has been added to the frame.

Since every frame include a size indication (32bits) I’m not sure what it is for, but the parser (of id3v2, rockbox, and eyeD3 at least) skips 4 bytes and continue parsing the frame.

TPE1 frame is 4 bytes, and TRCK is 2 bytes, so this obviously fails.

Clearing this bit should solve this file’s problem, but I think your fix is the proper one to deal with broen/damaged files.

This would help as well, but maybe would not be enough if the frame is a text frame and the size is 5 (so we only have the encoding byte remaining, and no proper data)

iso_decode() would be called with count == 0 so I guess your fix is needed anyway.

diff –git a/apps/metadata/mp3.c b/apps/metadata/mp3.c
index 0a4592b..c05e15c 100644
— a/apps/metadata/mp3.c
+++ b/apps/metadata/mp3.c
@@ -817,7 +817,8 @@ static void setid3v2title(int fd, struct mp3entry *entry)

                       return;
                   /* We don't need the data length */

- framelen -= 4;
+ if(framelen > 4) /* we need at least 1 data byte */
+ framelen -= 4;

               }
           }
       }

The data length is essentially the size of the frame in memory, before (or after, when reading) applying unsynchronization, compression and similar. Since we only read what we can fit in a fixed-size buffer, we don’t need that value.

Not sure how to handle this in the best way, but I think the fix should be in mp3.c. Perhaps continue with the next frame if framelen zero, exit if negative.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing