Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • Category Plugins
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.9
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by pondlife - 2011-08-28
Last edited by Buschel - 2011-12-22

FS#12249 - pdbox - Buffer access out-of-bounds: aiffhdr.a_samprate

cppcheck found an error in the line:

  	memcpy(aiffhdr->a_samprate, dogdoo, sizeof(dogdoo));

dogdoo is 14 bytes, but a_samprate is only 10 bytes.

Not sure of the correct fix so I’m bugging it up for an AIFF expert to sort out.

Closed by  Buschel
2011-12-22 19:18
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

Submitted with r30944

MikeS commented on 2011-08-28 15:10

AIFF samplerate is supposed to be an 80-bit IEEE floating point number, so 10 bytes sounds right, but who knows what dogdoo is in this context. :-)

The code is strange but seems to work as intended. aiffhdr is pointing to headerbuf[0], whereas headerbuf contains aiffhdr + additional first 8 bytes of datachunk (see line 141). The data chunk ID ‘SSND’ (see line 110) is exactly the last 4 bytes of dogdoo (see line 686). This also explains the weird code sequence in line 702 (”memcpy(aiffhdr→a_samprate + sizeof(dogdoo), &longtmp, 4);”) which is used to set the size of the data chunk.

Does this need to be so “optimised” - or could it be safely rewritten as 2 memsets?

Of course this can be re-written. The best would be to let it rewrite by someone who uses pdbox and can test the change… (I am not using it and do not plan to ever use it).

Me neither… I was aiming my message at any pdbox expert (or even user) who might read this in future.

I could not keep my fingers off this…

Applied the patch, but there is another trouble with the AIFF loader, so I’ll try to figure this issue out.

wincent, any update on this?

I do not think that the patch is harmful, I will be completely sure only when the AIFF loader works correctly. But, in my opinion the patch can be committed already. Shall I do this, or is there any objection?

I do not object any submit, but I would prefer to have a fix…

Me too, but currently I do not have much time available to fix the loader. On other hand, when committed, you would have one error less when using cppcheck.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing