[Vm-dev] Re: VMMaker-eem.163 fixes old SoundPlugin bug
David T. Lewis
lewis at mail.msen.com
Tue Mar 30 00:18:40 UTC 2010
That sounds good. And as long as you both are looking at
snd_RecordSamplesIntoAtLength(), can you please help me fix the
broken method signatures? This is to resolve pointer-as-int problems
that prevent sound from working on 64-bit platforms.
I have attached the updated platforms/Cross/plugins/SoundPlugin/SoundPlugin.h
file that needs to be checked in to Subversion (Andreas, can do this?).
I don't have Windows platform patches, but the updates are straightforward,
and the unix platforms patches are attached here:
http://lists.squeakfoundation.org/pipermail/vm-dev/2009-December/003586.html
The original Mantis issue is:
http://bugs.squeak.org/view.php?id=7103
The VMMaker patches are already in place.
Thanks!
Dave
On Mon, Mar 29, 2010 at 12:20:09PM -0700, Andreas Raab wrote:
>
> +1. The easiest short-term way to address this issue is to move the
> computation into the primitive and simply call the support function
> always with zero offset. That'll work regardless of whether the support
> code subtracts it and regardless of whether we want to change the
> interface in the future.
>
> Cheers,
> - Andreas
>
> On 3/29/2010 12:05 PM, Eliot Miranda wrote:
> >
> >
> >
> >
> >Hi All,
> >
> > it turns out I jumped the gun on this. We have been hunting a
> >buffer overrun bug and I mistakenly thought this was it. In fact our
> >bug was in one of the media codec support libraries we use from a
> >third-party.
> >
> >I still think the code is confusing, and it looks to me as if it is
> >broken on Mac OS and correct, but confusing, on win32. Anyone who wants
> >to review this /please/ do. Here's the original code:
> >
> >SoundPlugin>>primitiveSoundRecordSamplesInto: buf startingAt:
> >startWordIndex
> >"Record a buffer's worth of 16-bit sound samples."
> >| bufSizeInBytes samplesRecorded |
> >self primitive: 'primitiveSoundRecordSamples'
> >parameters: #(WordArray SmallInteger ).
> >
> >interpreterProxy failed ifFalse:
> >[bufSizeInBytes := (interpreterProxy slotSizeOf: buf cPtrAsOop) * 4.
> >interpreterProxy success: (startWordIndex >= 1 and: [startWordIndex - 1
> >* 2 < bufSizeInBytes])].
> >
> >interpreterProxy failed ifFalse:
> >[samplesRecorded := self cCode: 'snd_RecordSamplesIntoAtLength((int)buf,
> >startWordIndex - 1, bufSizeInBytes)'].
> >^ samplesRecorded asPositiveIntegerObj
> >
> >My objection in the above is that we do the bounds check above but leave
> >it up to the support code snd_RecordSamplesIntoAtLength to derive the
> >start pointer buf + ((startWordIndex - 1) * 2). IMO these two belong
> >together.
> >
> >Now in the Mac code there is no code to subtract (startWordIndex - 1) *
> >2 from bufSizeInBytes, and so without my fix there could be a buffer
> >overrun:
> >
> >int snd_RecordSamplesIntoAtLength(int buf, int startSliceIndex, int
> >bufferSizeInBytes) {
> > /* if data is available, copy as many sample slices as possible
> >into the
> > given buffer starting at the given slice index. do not write
> >past the
> > end of the buffer, which is buf + bufferSizeInBytes. return the
> >number
> > of slices (not bytes) copied. a slice is one 16-bit sample in mono
> > or two 16-bit samples in stereo. */
> > int bytesPerSlice = (recordBuffer1.brokenOSXWasMono) ? 2 :
> >((recordBuffer1.stereo) ? 4 : 2);
> > char *nextBuf = (char *) buf + (startSliceIndex * bytesPerSlice);
> > char *bufEnd = (char *) buf + bufferSizeInBytes;
> > char *src, *srcEnd;
> > RecordBuffer recBuf = nil;
> > int bytesCopied;
> >
> >On win32 there is code to do the subtraction. e.g. looking at the
> >Direct Sound side of the code:
> >
> >int dx_snd_RecordSamplesIntoAtLength(int buf, int startSliceIndex, int
> >bufferSizeInBytes) {
> > /* if data is available, copy as many sample slices as possible into the
> > given buffer starting at the given slice index. do not write past the
> > end of the buffer, which is buf + bufferSizeInBytes. return the
> > number
> > of slices (not bytes) copied. a slice is one 16-bit sample in mono
> > or two 16-bit samples in stereo. */
> > int bytesPerSlice = (waveInFormat.nChannels * 2);
> > int bytesCopied;
> > char *srcPtr, *srcPtr2, *dstPtr;
> > HRESULT hRes;
> > DWORD srcLen, srcLen2;
> >
> >...
> > /* Figure out how much data we want to copy (don't overflow either
> >the source or destination buffers. */
> > bytesCopied = bufferSizeInBytes - (startSliceIndex * bytesPerSlice);
> > if(bytesCopied > recBufferAvailable)
> > bytesCopied = recBufferAvailable;
> >
> > /* Lock the portion of the DirectSound buffer that we will copy data
> >from. */
> > hRes = IDirectSoundCaptureBuffer_Lock(lpdRecBuffer,
> > recBufferReadPosition,
> > bytesCopied,
> > (void*)&srcPtr, &srcLen,
> > (void*)&srcPtr2, &srcLen2,
> > 0);
> >
> >So this is an example of a really confusing API. If instead the API were
> >snd_RecordSamplesIntoAtLength(int buf, int bufferSizeInBytes)
> >and the primitive read
> >
> >SoundPlugin>>primitiveSoundRecordSamplesInto: buf startingAt:
> >startWordIndex
> >"Record a buffer's worth of 16-bit sound samples."
> >| bufSizeInBytes samplesRecorded startByteIndex |
> >self primitive: 'primitiveSoundRecordSamples'
> >parameters: #(WordArray SmallInteger ).
> >
> >interpreterProxy failed ifFalse:
> >[bufSizeInBytes := (interpreterProxy slotSizeOf: buf cPtrAsOop) * 4.
> >startByteIndex := startWordIndex - 1 * .
> >interpreterProxy success: (startWordIndex >= 1 and: [startByteIndex <
> >bufSizeInBytes])].
> >
> >interpreterProxy failed ifFalse:
> >[samplesRecorded := self cCode: 'snd_RecordSamplesIntoAtLength((int)buf
> >+ startByteIndex, bufSizeInBytes - startByteIndex'].
> >^ samplesRecorded asPositiveIntegerObj
> >
> >there would be much less chance for (as the Fat Controller says)
> >confusion and delay.
> >
> >
> >cheers
> >Eliot
> >On Mon, Mar 22, 2010 at 5:18 PM, Eliot Miranda <eliot.miranda at gmail.com
> ><mailto:eliot.miranda at gmail.com>> wrote:
> >
> > Use of primitiveSoundRecordSamples with a start index other than 1
> > could corrupt the heap if enough samples were recorded.
> >
> > Name: VMMaker-eem.163
> > Author: eem
> > Time: 22 March 2010, 5:15:57 pm
> > UUID: 2fa86dda-e18c-48cd-bcac-856504aba8dc
> > Ancestors: VMMaker-ar.162
> >
> > SoundPlugin: fix bounds bug in primitiveSoundRecordSamples. The
> > call to snd_RecordSamplesIntoAtLength neglected to subtract the
> > start index from the size of the buffer to be written into.
> >
> > best
> > Eliot
> >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SoundPlugin.h
Type: text/x-chdr
Size: 1244 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20100329/d0cecaf9/SoundPlugin.h
More information about the Vm-dev
mailing list