VMMaker-eem.163 fixes old SoundPlugin bug

>
> +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 <[hidden email]
> ><mailto:[hidden email]>> 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
> >
> >

> 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