Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11948 - Issues reported by valgrind

Attached to Project: Rockbox
Opened by Thomas Jarosch (thomasjfox) - Wednesday, 16 February 2011, 21:28 GMT+2
Last edited by Andree Buschmann (Buschel) - Thursday, 17 February 2011, 19:36 GMT+2
Task Type Bugs
Category ID3 / meta data
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.7.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Hello,

attached are four small issues identified by valgrind-3.6.0
using the rockbox SDL app build from r29321:

1.
==9141== Conditional jump or move depends on uninitialised value(s)
==9141== at 0x410221: get_mp3file_info (mp3data.c:467)
==9141== by 0x456555: getsonglength (mp3.c:67)
==9141== by 0x4568D9: get_mp3_metadata (mp3.c:176)
==9141== by 0x45429F: get_metadata (metadata.c:330)
==9141== by 0x4630DC: add_tagcache (tagcache.c:1852)
==9141== by 0x46715D: check_dir (tagcache.c:4366)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x46732A: tagcache_build (tagcache.c:4446)
==9141== by 0x4674E5: tagcache_thread (tagcache.c:4582)

2.
==9141== Conditional jump or move depends on uninitialised value(s)
==9141== at 0x45999F: read_mp4_atom (mp4.c:140)
==9141== by 0x45A638: read_mp4_container (mp4.c:555)
==9141== by 0x45A7DA: read_mp4_container (mp4.c:593)
==9141== by 0x45A7DA: read_mp4_container (mp4.c:593)
==9141== by 0x45AC80: get_mp4_metadata (mp4.c:773)
==9141== by 0x45429F: get_metadata (metadata.c:330)
==9141== by 0x4630DC: add_tagcache (tagcache.c:1852)
==9141== by 0x46715D: check_dir (tagcache.c:4366)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x46732A: tagcache_build (tagcache.c:4446)

3.
==9141== Conditional jump or move depends on uninitialised value(s)
==9141== at 0x4599C5: read_mp4_atom (mp4.c:150)
==9141== by 0x45A638: read_mp4_container (mp4.c:555)
==9141== by 0x45A7DA: read_mp4_container (mp4.c:593)
==9141== by 0x45A7DA: read_mp4_container (mp4.c:593)
==9141== by 0x45AC80: get_mp4_metadata (mp4.c:773)
==9141== by 0x45429F: get_metadata (metadata.c:330)
==9141== by 0x4630DC: add_tagcache (tagcache.c:1852)
==9141== by 0x46715D: check_dir (tagcache.c:4366)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x46732A: tagcache_build (tagcache.c:4446)

4.
==9141== Conditional jump or move depends on uninitialised value(s)
==9141== at 0x45A644: read_mp4_container (mp4.c:562)
==9141== by 0x45A7DA: read_mp4_container (mp4.c:593)
==9141== by 0x45A7DA: read_mp4_container (mp4.c:593)
==9141== by 0x45AC80: get_mp4_metadata (mp4.c:773)
==9141== by 0x45429F: get_metadata (metadata.c:330)
==9141== by 0x4630DC: add_tagcache (tagcache.c:1852)
==9141== by 0x46715D: check_dir (tagcache.c:4366)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x467121: check_dir (tagcache.c:4359)
==9141== by 0x46732A: tagcache_build (tagcache.c:4446)
==9141== by 0x4674E5: tagcache_thread (tagcache.c:4582)

Could someone with knowledge of the specific parsing code take a look
if this is valid or a false positive?

Wild guess: Files with corrupted metadata and we don't do proper error checking
in some corner cases.

Thanks,
Thomas
This task depends upon

Closed by  Andree Buschmann (Buschel)
Thursday, 17 February 2011, 19:36 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  Fixed with r29323.
Comment by Andree Buschmann (Buschel) - Wednesday, 16 February 2011, 23:10 GMT+2
Issues 2-4 could occur due to missing initialization of two variables (see mp4_v01.patch).

I am not sure about issue 1. To me it seems like "vbrheader[]" needs to be converted to "int" before left-shifting. Also "frame[]" is not initialized with zero. mp3data_v01.patch does both.

Can you please check again with both patches applied?
   mp4_v01.patch (0.5 KiB) 
 apps/metadata/mp4.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

   mp3data_v01.patch (1.3 KiB) 
 apps/mp3data.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comment by Thomas Jarosch (thomasjfox) - Thursday, 17 February 2011, 10:21 GMT+2
Good catch! With both patches applied, the issues are gone.
(rebuilt the database early this morning)


btw: I've found another uninitialized variables issue if you press rew/ff really fast.
Might be related to the rew/ff lockups mentioned on IRC.
I'll open a separate ticket for that the next days.

Loading...