- Status Closed
- Percent Complete
- 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
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
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
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
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.