Hi All,<div><br></div><div> 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.</div>
<div><br></div><div>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:</div>
<div><br></div><div>SoundPlugin>>primitiveSoundRecordSamplesInto: buf startingAt: startWordIndex <div><span class="Apple-tab-span" style="white-space:pre"> </span>"Record a buffer's worth of 16-bit sound samples."</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>| bufSizeInBytes samplesRecorded |</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>self primitive: 'primitiveSoundRecordSamples'</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>parameters: #(WordArray SmallInteger ).</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>interpreterProxy failed ifFalse:</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>[bufSizeInBytes := (interpreterProxy slotSizeOf: buf cPtrAsOop) * 4.</div><div><span class="Apple-tab-span" style="white-space: pre; "></span></div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>interpreterProxy success: (startWordIndex >= 1 and: [startWordIndex - 1 * 2 < bufSizeInBytes])].</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>interpreterProxy failed ifFalse:</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>[samplesRecorded := self cCode: 'snd_RecordSamplesIntoAtLength((int)buf, startWordIndex - 1, bufSizeInBytes)'].</div>
<div><span class="Apple-tab-span" style="white-space: pre; "></span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>^ samplesRecorded asPositiveIntegerObj</div><div><br></div><div>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.</div>
<div><br></div><div>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:</div><div><br></div><div><div>int snd_RecordSamplesIntoAtLength(int buf, int startSliceIndex, int bufferSizeInBytes) {</div>
<div> /* if data is available, copy as many sample slices as possible into the</div><div> given buffer starting at the given slice index. do not write past the</div><div> end of the buffer, which is buf + bufferSizeInBytes. return the number</div>
<div> of slices (not bytes) copied. a slice is one 16-bit sample in mono</div><div> or two 16-bit samples in stereo. */</div><div> int bytesPerSlice = (recordBuffer1.brokenOSXWasMono) ? 2 : ((recordBuffer1.stereo) ? 4 : 2);</div>
<div> char *nextBuf = (char *) buf + (startSliceIndex * bytesPerSlice);</div><div> char *bufEnd = (char *) buf + bufferSizeInBytes;</div><div> char *src, *srcEnd;</div><div> RecordBuffer recBuf = nil;</div><div>
int bytesCopied;</div><div><br></div><div>On win32 there is code to do the subtraction. e.g. looking at the Direct Sound side of the code:</div><div><br></div><div><div>int dx_snd_RecordSamplesIntoAtLength(int buf, int startSliceIndex, int bufferSizeInBytes) {</div>
<div> /* if data is available, copy as many sample slices as possible into the</div><div> given buffer starting at the given slice index. do not write past the</div><div> end of the buffer, which is buf + bufferSizeInBytes. return the number</div>
<div> of slices (not bytes) copied. a slice is one 16-bit sample in mono</div><div> or two 16-bit samples in stereo. */</div><div> int bytesPerSlice = (waveInFormat.nChannels * 2);</div><div> int bytesCopied;</div>
<div> char *srcPtr, *srcPtr2, *dstPtr;</div><div> HRESULT hRes;</div><div> DWORD srcLen, srcLen2;</div><div><br></div><div>...</div><div><div> /* Figure out how much data we want to copy (don't overflow either the source or destination buffers. */</div>
<div> bytesCopied = bufferSizeInBytes - (startSliceIndex * bytesPerSlice);</div><div> if(bytesCopied > recBufferAvailable)</div><div> bytesCopied = recBufferAvailable;</div><div><br></div><div> /* Lock the portion of the DirectSound buffer that we will copy data from. */</div>
<div> hRes = IDirectSoundCaptureBuffer_Lock(lpdRecBuffer,</div><div> recBufferReadPosition,</div><div> bytesCopied,</div><div> (void*)&srcPtr, &srcLen,</div>
<div> (void*)&srcPtr2, &srcLen2,</div><div> 0);</div><div><br></div><div>So this is an example of a really confusing API. If instead the API were</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>snd_RecordSamplesIntoAtLength(int buf, int bufferSizeInBytes)</div>
</div></div></div>and the primitive read</div><div><br></div><div>SoundPlugin>>primitiveSoundRecordSamplesInto: buf startingAt: startWordIndex <div><span class="Apple-tab-span" style="white-space: pre; "> </span>"Record a buffer's worth of 16-bit sound samples."</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span>| bufSizeInBytes samplesRecorded startByteIndex |</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>self primitive: 'primitiveSoundRecordSamples'</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span>parameters: #(WordArray SmallInteger ).</div><div><br></div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>interpreterProxy failed ifFalse:</div>
<div><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>[bufSizeInBytes := (interpreterProxy slotSizeOf: buf cPtrAsOop) * 4.</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>startByteIndex := startWordIndex - 1 * .</div>
<div><span class="Apple-tab-span" style="white-space: pre; "></span></div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>interpreterProxy success: (startWordIndex >= 1 and: [startByteIndex < bufSizeInBytes])].</div>
<div><br></div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>interpreterProxy failed ifFalse:</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span><span class="Apple-tab-span" style="white-space: pre; "> </span>[samplesRecorded := self cCode: 'snd_RecordSamplesIntoAtLength((int)buf + startByteIndex, bufSizeInBytes - startByteIndex'].</div>
<div><span class="Apple-tab-span" style="white-space: pre; "></span></div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>^ samplesRecorded asPositiveIntegerObj</div><div><br></div><div>there would be much less chance for (as the Fat Controller says) confusion and delay.</div>
<div><br></div><div><br></div><div>cheers</div><div>Eliot</div><div class="gmail_quote">On Mon, Mar 22, 2010 at 5:18 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div>Use of primitiveSoundRecordSamples with a start index other than 1 could corrupt the heap if enough samples were recorded.</div>
<div><br></div><div>Name: VMMaker-eem.163</div><div>Author: eem</div><div>Time: 22 March 2010, 5:15:57 pm</div>
<div>UUID: 2fa86dda-e18c-48cd-bcac-856504aba8dc</div><div>Ancestors: VMMaker-ar.162</div><div><br></div><div>SoundPlugin: fix bounds bug in primitiveSoundRecordSamples. The</div><div>call to snd_RecordSamplesIntoAtLength neglected to subtract the</div>
<div>start index from the size of the buffer to be written into.</div><div><br></div><div>best</div><div>Eliot</div>
</blockquote></div><br></div>