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

Attached to Project: Rockbox
Opened by Steve Bavin (pondlife) - Sunday, 28 August 2011, 09:58 GMT
Last edited by Andree Buschmann (Buschel) - Thursday, 22 December 2011, 19:18 GMT
Task Type Bugs
Category Plugins
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.9
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


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

Closed by  Andree Buschmann (Buschel)
Thursday, 22 December 2011, 19:18 GMT
Reason for closing:  Accepted
Additional comments about closing:  Submitted with r30944
Comment by Michael Sevakis (MikeS) - Sunday, 28 August 2011, 15:10 GMT
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. :-)
Comment by Andree Buschmann (Buschel) - Sunday, 28 August 2011, 22:12 GMT
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.
Comment by Steve Bavin (pondlife) - Monday, 29 August 2011, 10:47 GMT
Does this need to be so "optimised" - or could it be safely rewritten as 2 memsets?
Comment by Andree Buschmann (Buschel) - Monday, 29 August 2011, 13:51 GMT
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).
Comment by Steve Bavin (pondlife) - Tuesday, 30 August 2011, 07:37 GMT
Me neither... I was aiming my message at any pdbox expert (or even user) who might read this in future.
Comment by Andree Buschmann (Buschel) - Tuesday, 30 August 2011, 11:13 GMT
I could not keep my fingers off this...
Comment by Wincent Balin (wincent) - Monday, 24 October 2011, 23:26 GMT
Applied the patch, but there is another trouble with the AIFF loader, so I'll try to figure this issue out.
Comment by Andree Buschmann (Buschel) - Monday, 07 November 2011, 19:30 GMT
wincent, any update on this?
Comment by Wincent Balin (wincent) - Monday, 07 November 2011, 19:47 GMT
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?
Comment by Andree Buschmann (Buschel) - Monday, 07 November 2011, 19:53 GMT
I do not object any submit, but I would prefer to have a fix...
Comment by Wincent Balin (wincent) - Monday, 07 November 2011, 19:57 GMT
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.