EZ-USB FX3 CyUSB .Net Heap Corruption

Tip / Sign in to post questions, reply, level up, and achieve exciting badges. Know more

cross mob
Anonymous
Not applicable

The current version of CyUSB.dll in the EZ-USB FX3 SDK has a serious bug. Also, the version of the dll is lower than previous versions (from 3.4.7 to 1.2.3) which causes all kinds of installer upgrade problems (Windows won't overwrite the file because the version of the dll is lower than the previous version).

The bug is in the CyUSBEndPoint.xferData method. The 3 managed buffers used are not pinned for the duration of the method. Therefore, the managed garbage collector can move the managed buffers during the async native operation, corrupting the managed heap. I've decompiled version 3.4.7 to verify that it was implemented correctly (pinning all 3 managed buffers). I also decompiled version 1.2.2. to verify that it only pins the overlap buffer (not the single transfer and data buffer). This seems like a serious regression.

Message was edited by: Dan Albertson (fixed typos)

1 Solution

Hi Dan,

Thanks for pointing out this issue.

You are right, you have to pin the buffers using GCHandle.Alloc() to protect them from the garbage collector and free it manually later.

You can refer to the implementation of Streamer example there we have used this work around. I am posting that code snippet for your reference. For more information you can check the LockNLoad function implementation in the Streamer example.

bufSingleTransfer = GCHandle.Alloc(cBufs, GCHandleType.Pinned);

                bufDataAllocation = GCHandle.Alloc(xBufs, GCHandleType.Pinned);

                bufPktsInfo = GCHandle.Alloc(pktsInfo, GCHandleType.Pinned);

                handleOverlap = GCHandle.Alloc(oLaps, GCHandleType.Pinned);

                // oLaps "fixed" keyword variable is in use. So, we are good.

                /////////////////////////////////////////////////////////////////////////////////////////////           

                unsafe

                {

                    //fixed (byte* tL0 = oLaps)

                    {

                        CyUSB.OVERLAPPED ovLapStatus = new CyUSB.OVERLAPPED();

                        ovLapStatus = (CyUSB.OVERLAPPED)Marshal.PtrToStructure(handleOverlap.AddrOfPinnedObject(),                               typeof(CyUSB.OVERLAPPED));

                        ovLapStatus.hEvent = (IntPtr)PInvoke.CreateEvent(0, 0, 0, 0);

                        Marshal.StructureToPtr(ovLapStatus, handleOverlap.AddrOfPinnedObject(), true);

                        // Pre-load the queue with a request

                        int len = BufSz;

                        if (EndPoint.BeginDataXfer(ref cBufs, ref xBufs, ref len, ref oLaps) == false)

                            Failures++;

                    }

                    j++;

                }

            }

            XferData(cBufs, xBufs, oLaps, pktsInfo, handleOverlap);          // All loaded. Let's go! // implemented in the example itself

Thanks & Regards
Abhinav

View solution in original post

8 Replies
Anonymous
Not applicable

The decompiled version 3.4.7 of xferData shows that all three managed buffers are pinned for the duration of the method:

public virtual unsafe bool XferData(ref byte[] buf, ref int len)

    {

      byte[] ov = new byte[this.OverlapSignalAllocSize];

      fixed (byte* numPtr1 = ov)

      {

        ((OVERLAPPED*) numPtr1)->hEvent = PInvoke.CreateEvent(0U, 0U, 0U, 0U);

        byte[] singleXfer = new byte[38 + (this.XferMode == XMODE.DIRECT ? 0 : len)];

        fixed (byte* numPtr2 = singleXfer)

          fixed (byte* numPtr3 = buf)

          {

            int typeCode1 = (int) numPtr2->GetTypeCode();

            int typeCode2 = (int) numPtr3->GetTypeCode();

            this.BeginDataXfer(ref singleXfer, ref buf, ref len, ref ov);

            bool flag1 = this.WaitForIO(((OVERLAPPED*) numPtr1)->hEvent);

            bool flag2 = this.FinishDataXfer(ref singleXfer, ref buf, ref len, ref ov);

            PInvoke.CloseHandle(((OVERLAPPED*) numPtr1)->hEvent);

            if ((!flag1 || !flag2) && (this._lastError != 0U && this._usbdStatus == 0U))

              this._usbdStatus = uint.MaxValue;

            return flag1 && flag2;

          }

      }

    }

The 1.2.2 version decompiled shows only the overlap buffer pinned for the duration

public virtual unsafe bool XferData(ref byte[] buf, ref int len)

    {

      byte[] ov = new byte[this.OverlapSignalAllocSize];

      fixed (byte* numPtr = ov)

      {

        ((OVERLAPPED*) numPtr)->hEvent = PInvoke.CreateEvent(0U, 0U, 0U, 0U);

        byte[] singleXfer = new byte[38 + (this.XferMode == XMODE.DIRECT ? 0 : len)];

        byte[] numArray1;

        if ((numArray1 = singleXfer) != null)

        {

          int length1 = numArray1.Length;

        }

        byte[] numArray2;

        if ((numArray2 = buf) != null)

        {

          int length2 = numArray2.Length;

        }

        this.BeginDataXfer(ref singleXfer, ref buf, ref len, ref ov);

        bool flag1 = this.WaitForIO(((OVERLAPPED*) numPtr)->hEvent);

        bool flag2 = this.FinishDataXfer(ref singleXfer, ref buf, ref len, ref ov);

        PInvoke.CloseHandle(((OVERLAPPED*) numPtr)->hEvent);

        return flag1 && flag2;

      }

    }

Anonymous
Not applicable

Ran into the same issue - kept getting random fatal errors that would crash my application regardless of any try...catch block. Took me forever to track down, and I only found it because I happened to want to make improvements to our wrapper around CyUSB.NET. As a work around, my wrapper pins any buffers I send to CyUSB. As you say, this is a serious bug - and one that is easy to reproduce with a bunch of asynchronous I/O activity. The fact that this got released makes one question exactly how much testing they are putting these releases through - as this would be easy to catch with some routine test code. Much harder to isolate down stream in the client application.

Anonymous
Not applicable

@ryan.williams_3641181, be careful. It's not sufficient to only pin the buffer you pass to the xferData method. The method itself creates two managed buffers (singleXfer and ov) which need to be pinned as well. The ov buffer does get a fixed pointer for the duration of the method. However, the data buffer and the singleXfer buffers do not. I ended up downloading the source code, creating a local repo for just us, fixing the code, and rebuilding the library. If you don't want to do that, you could just decompile the library, copy the code for the xferData method, create an extension method (called xferDataPinned, example) where you pin all three buffers. If you look at their source code, they try pinning all three buffers using a fixed pointer. The problem is that they don't reference the fixed pointers for the singleXfer or the data buffer you passed in. So, when it was built in release, the optimizer removes those fixed pointers (data and singleXfer). When I fixed this method, I explicitly added var handle = GCHandle.Alloc(managedBuffer, GCHandleType.Pinned) for each of the three managed buffers, then did a handle.Free() at the end of the method for each of the three handles.

This took over a week of my time near a release (and probably a few years off my life).

Hi Dan,

Here is the source code of XferData in CyAPI. This code doesn't match with the one that you had posted. Could you please point to the source file that you are referring to.

I don't find any potential issue with this code.

public unsafe override bool XferData(ref  byte[] buf, ref Int32 len)

        {

            byte[] ovLap = new byte[OverlapSignalAllocSize];

            fixed (byte* tmp0 = ovLap)

            {

                OVERLAPPED* ovLapStatus = (OVERLAPPED*)tmp0;

                ovLapStatus->hEvent = PInvoke.CreateEvent(0, 0, 0, 0);

                // This SINGLE_TRANSFER buffer must be allocated at this level.

                int bufSz = CyConst.SINGLE_XFER_LEN + GetPktBlockSize(len) + ((XferMode == XMODE.DIRECT) ? 0 : len);

                byte[] cmdBuf = new byte[bufSz];

                fixed (byte* tmp1 = cmdBuf, tmp2 = buf)

                {

                    bool bResult = BeginDataXfer(ref cmdBuf, ref buf, ref len, ref ovLap);

                    bool wResult = WaitForIO(ovLapStatus->hEvent);

                    bool fResult = FinishDataXfer(ref cmdBuf, ref buf, ref len, ref ovLap);

                    PInvoke.CloseHandle(ovLapStatus->hEvent);

                    return wResult && fResult;

                }

            }

        }

Correct me if I am wrong.

Thanks  & Regards

Abhinav

Anonymous
Not applicable

Hi Abhinav,

The code I posted came from a decompiler tool (like DotPeek or .NET Reflector; In my case I used DotPeek and decompiled the current CyUSB 1.2.2). I agree that what you posted is the current code in the CyUSB API. The problem lies in this block:

fixed (byte* tmp1 = cmdBuf, tmp2 = buf)

                {

                    bool bResult = BeginDataXfer(ref cmdBuf, ref buf, ref len, ref ovLap);

                    bool wResult = WaitForIO(ovLapStatus->hEvent);

                    bool fResult = FinishDataXfer(ref cmdBuf, ref buf, ref len, ref ovLap);

                    PInvoke.CloseHandle(ovLapStatus->hEvent);

                    return wResult && fResult;

                }

The issue here is that although a fixed pointer was created for those two managed arrays (tmp1 and tmp2), they are never referenced anywhere inside that fixed block. So, when the code is compiled in release, the optimizer will strip those fixed pointers away. The compiler's optimizer basically says "I don't see a reference anywhere to those fixed pointers (tmp1 and tmp2), so I'm not going to waste time allocating fixed pointers for them". You can see that is the case from the decompiled code that I pasted above.

In the decompiled version of 3.4.7 code I pasted above, you can see that it worked correctly because the two fixed pointers where referenced inside their fixed blocks (I've put the lines of interest in bold):

     fixed (byte* numPtr2 = singleXfer)

          fixed (byte* numPtr3 = buf)

          {

            int typeCode1 = (int) numPtr2->GetTypeCode();   <--------HERE

            int typeCode2 = (int) numPtr3->GetTypeCode();   <--------AND HERE

            this.BeginDataXfer(ref singleXfer, ref buf, ref len, ref ov);

            bool flag1 = this.WaitForIO(((OVERLAPPED*) numPtr1)->hEvent);

            bool flag2 = this.FinishDataXfer(ref singleXfer, ref buf, ref len, ref ov);

            PInvoke.CloseHandle(((OVERLAPPED*) numPtr1)->hEvent);

            if ((!flag1 || !flag2) && (this._lastError != 0U && this._usbdStatus == 0U))

              this._usbdStatus = uint.MaxValue;

            return flag1 && flag2;

          }

      }

In summary, it's not sufficient to create fixed pointers that never get referenced, because the optimizer will strip that away. You can either arbitrarily referenced those fixed pointers inside the fixed block (like is done in the 3.4.7 code), or you can use GCHandle.Alloc(managedBuffer, GCHandleType.Pinned), and then explicitly call free on that object at the end of the method.

Thanks for reaching out. Let me know if any of this doesn't make sense.

Dan Albertson

0 Likes

Hi Dan,

Thanks for pointing out this issue.

You are right, you have to pin the buffers using GCHandle.Alloc() to protect them from the garbage collector and free it manually later.

You can refer to the implementation of Streamer example there we have used this work around. I am posting that code snippet for your reference. For more information you can check the LockNLoad function implementation in the Streamer example.

bufSingleTransfer = GCHandle.Alloc(cBufs, GCHandleType.Pinned);

                bufDataAllocation = GCHandle.Alloc(xBufs, GCHandleType.Pinned);

                bufPktsInfo = GCHandle.Alloc(pktsInfo, GCHandleType.Pinned);

                handleOverlap = GCHandle.Alloc(oLaps, GCHandleType.Pinned);

                // oLaps "fixed" keyword variable is in use. So, we are good.

                /////////////////////////////////////////////////////////////////////////////////////////////           

                unsafe

                {

                    //fixed (byte* tL0 = oLaps)

                    {

                        CyUSB.OVERLAPPED ovLapStatus = new CyUSB.OVERLAPPED();

                        ovLapStatus = (CyUSB.OVERLAPPED)Marshal.PtrToStructure(handleOverlap.AddrOfPinnedObject(),                               typeof(CyUSB.OVERLAPPED));

                        ovLapStatus.hEvent = (IntPtr)PInvoke.CreateEvent(0, 0, 0, 0);

                        Marshal.StructureToPtr(ovLapStatus, handleOverlap.AddrOfPinnedObject(), true);

                        // Pre-load the queue with a request

                        int len = BufSz;

                        if (EndPoint.BeginDataXfer(ref cBufs, ref xBufs, ref len, ref oLaps) == false)

                            Failures++;

                    }

                    j++;

                }

            }

            XferData(cBufs, xBufs, oLaps, pktsInfo, handleOverlap);          // All loaded. Let's go! // implemented in the example itself

Thanks & Regards
Abhinav

Anonymous
Not applicable

Abhinav,

I've posted the updated code I'm using for XferData below. The overlap buffer as a fixed pointer should be fine, as it's referenced inside the fixed block. However, I've created pinned pointers for the command buffer and the data buffer.

public unsafe virtual bool XferData(ref byte[] buf, ref int len)

        {

            byte[] ovLap = new byte[OverlapSignalAllocSize];

            fixed (byte* fixedOvLap = ovLap)

            {

                OVERLAPPED* ovLapStatus = (OVERLAPPED*)fixedOvLap;

                ovLapStatus->hEvent = PInvoke.CreateEvent(0, 0, 0, 0);

                // This SINGLE_TRANSFER buffer must be allocated at this level.

                int bufSz = CyConst.SINGLE_XFER_LEN + ((XferMode == XMODE.DIRECT) ? 0 : len);

                byte[] cmdBuf = new byte[bufSz];

                // These pinned pointers ensure that the buffers don't move in memory

                // While we're doing the asynchronous IO - Begin/Wait/Finish

                var cmd_buff_handle = GCHandle.Alloc(cmdBuf, GCHandleType.Pinned);

                var data_handle = GCHandle.Alloc(buf, GCHandleType.Pinned);

                bool bResult = BeginDataXfer(ref cmdBuf, ref buf, ref len, ref ovLap);

                //

                //  This waits for driver to call IoRequestComplete on the IRP

                //  we just sent.

                //

                bool wResult = WaitForIO(ovLapStatus->hEvent);

                bool fResult = FinishDataXfer(ref cmdBuf, ref buf, ref len, ref ovLap);

                PInvoke.CloseHandle(ovLapStatus->hEvent);

                cmd_buff_handle.Free();

                data_handle.Free();

                return wResult && fResult;

            }

        }

0 Likes

Dan,

I think this is the right way of using "BeginDataXfer()". First of all you have to fix the cmdBuf & buf using GCHandle.Alloc() before passing it as an argument. Your problem is fixed with this workaround, correct me if I am wrong?

Thanks & Regards

Abhinav

0 Likes