Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9522 - Initializing database gives Data abort error

Attached to Project: Rockbox
Opened by Dan (dalesd) - Sunday, 02 November 2008, 05:19 GMT+2
Last edited by Magnus Holmgren (learman) - Tuesday, 04 November 2008, 21:02 GMT+2
Task Type Bugs
Category Database
Status Closed
Assigned To No-one
Player Type iPod 5G
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

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")
This task depends upon

Closed by  Magnus Holmgren (learman)
Tuesday, 04 November 2008, 21:02 GMT+2
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? :)
Comment by Dan (dalesd) - Sunday, 02 November 2008, 05:21 GMT+2
Here's the file responsible for the problem.
   SOIZ.mp3 (552.5 KiB)
Comment by Nils Wallménius (nls) - Sunday, 02 November 2008, 11:34 GMT+2
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"

   fs9522fix.diff (0.4 KiB) 
 firmware/common/unicode.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comment by Rafaël Carré (funman) - Sunday, 02 November 2008, 15:17 GMT+2
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.
Comment by Rafaël Carré (funman) - Sunday, 02 November 2008, 15:36 GMT+2
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;
}
}
}
Comment by Magnus Holmgren (learman) - Sunday, 02 November 2008, 18:31 GMT+2
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...