• TI thinks resolved

TM4C123GH6PM: usblib CDC driver transmits stack data (with possible corruption?)

Part Number: TM4C123GH6PM

Hi,

I'm working on a USB driver (for USBTMC) based on the usblib USBDCDC driver in TivaWare. I found what appears to be a bug in usbdcdc.c and I'd appreciate if it could be confirmed as a bug.

I'm using TivaWare 2.1.4.178.

In the HandleRequests(...) function, for USB_CDC_GET_LINE_CODING, USBDCDSendDataEP0() (line 1849) is called with data defined on the stack. The documencation for USBDCDSendDataEP0 says that the data must remain unchanged until pfnDataSent is received. But, since it's sending data on the stack, it seems like it could become corrupted before being sent.

Thanks,

Nathan

  • Hi Nathan,

    The *pui8Data is a pointer to the buffer of data to send via EP0. Normally this will be a buffer in the SRAM that already contains the data to be sent to the host. The application should already have the valid data ready to be sent pointed to by *pui8Data. I kind of doubt that there is a bug here even though I'm not a expert in the USB drivers. This USBDCDSendDataEP0 is used in various different places in different usb library files.

    regards,

    Charles

     

  • In reply to Charles Tsai:

    Hello Nathan,

    To add to Charles' answer, I think your source of concern may be coming from the declaration of the sLineCoding variable within the case statement?

    If so, I don't believe this should be any issue, because from the code below, it it only used as a pointer to point the USBDCDSendDataEP0 to the actual location of the line coding. That means it wouldn't be data passed onto the stack via the API, but just an address which after passed to the API would remain on the stack until the API accesses the data at that address like any standard pointer in a function call would behave. Therefore, the data would remain unchanged throughout the process as it isn't stored on the stack. It would only be accessed for transmission based on the memory location pointed to by the address.

                //
                // Ask the client for the current line coding.
                //
                psCDCDevice->pfnControlCallback(psCDCDevice->pvControlCBData,
                                             USBD_CDC_EVENT_GET_LINE_CODING, 0,
                                             &sLineCoding);

    I will also note that most other calls for the USBDCDSendDataEP0 also use pointers for similar purposes, so from what I can see, there isn't a bug or design flaw with the implementation.

    Further, the API for the USBDCDSendDataEP0 further enforces that the data is being provided to the API via pointer only based on this statement within the API description:

    //! \param pui8Data is a pointer to the buffer to send via endpoint zero.

    Does this information alleviate your concerns?

    Best Regards,

    Ralph Jacobi

  • In reply to Ralph Jacobi:

    Yes, my concern is that sLineCoding is declared within the case statement.

    The call to pfnControlCallback is perfectly fine, as it's still within the HandleRequests() function.

    The USBDCDSendDataEP0() function's documentation says "The contents of the \e pui8Data buffer must remain unchanged until the <tt>pfnDataSent</tt> callback is received.".

    &sLineCoding is used as pui8Data, so I think that it'll have to remain valid until pfnDataSent is called?

    As sLineCoding is declared within the HandleRequests() function, it resides on the stack. Once the function returns, the stack frame will be torn down, so the data at &sLineCoding may become corrupt if other functions are called and the stack is overwritten?

    Unless the documentation for the function is wrong and we don't need to wait for the pfnDataSent callback?

    -Nathan

  • In reply to Nathan Conrad71:

    Hello Nathan,

    The sLineCoding pointer is just a temporary means to pass an address. Once the USBDCDSendDataEP0 API is called, then the following code is executed:

    g_psDCDInst[0].pui8EP0Data = pui8Data;

    Once that call occurs, the address that points to the data (which is the only thing provided by sLineCoding) is stored within the usbdenum.c file in the global variable g_psDCDInst, and thus it is now outside of the stack. At this point, the address held on the stack by sLineCoding is not required anymore, so it doesn't matter if the stack frame is torn down while waiting for the pfnDataSent callback.

    Best Regards,

    Ralph Jacobi

  • In reply to Ralph Jacobi:

    Ralph,

    I'm still having trouble understanding this. Perhaps the code is right, so I'll sleep on it and look at it in the morning again.

    I'm not worried about the pointer pui8Data being lost, as it's saved in g_psDCDInst (as you said).

    I'm worried about the data that is being pointed to. As far as I can read, *pui8Data is stored in the stack.

    The issue is that sLineCoding isn't a pointer. It's a struct allocated on the stack (inside the case statement):

            case USB_CDC_GET_LINE_CODING:
            {
                // sLineCoding is stored in the stack.
                tLineCoding sLineCoding;
                MAP_USBDevEndpointDataAck(psInst->ui32USBBase, USB_EP_0, false);
                // Copy the linecoding data into the local struct
                psCDCDevice->pfnControlCallback(psCDCDevice->pvControlCBData,
                                             USBD_CDC_EVENT_GET_LINE_CODING, 0,
                                             &sLineCoding);
                // Send the address of sLineCoding, which is a pointer to data on the stack
                USBDCDSendDataEP0(0, (uint8_t *)&sLineCoding, sizeof(tLineCoding));
                break;
            }

    pfnControlCallback copies the data in the application's linecoding struct into the provided pointer to pre-allocated memory, which is the sLineCoding declared within HandleRequests(...). It isn't setting it to a pointer to the application's linecoding.

    -Nathan

  • In reply to Nathan Conrad71:

    Hello Nathan,

    Okay, I understand the concern you are raising fully now. I am looking into it further, stay tuned please.

    Best Regards,

    Ralph Jacobi

  • In reply to Ralph Jacobi:

    Thanks,

    I admit that it's unlikely that this would cause issues in practice. I think the most likely scenario would be if a 2nd interrupt handler was called immediately after the USB handler. Perhaps it could be tested by triggering a software interrupt with a large stack frame size (many local variables, with optimization turned off?) at the end of the HandleRequests function.

    I did notice that a few of the other drivers in usblib have this same issue, perhaps with greater consequences. Having the line coding data corrupted doesn't seem like a major issue for any application I can think of.

    -Nathan
  • In reply to Nathan Conrad71:

    Hello Nathan,

    I think perhaps there was a devil in the details which we overlooked, because I certainly agree now that your interpretation that the data would vanish once the stack frames are torn down (sorry for not coming to that conclusion earlier by the way). I was looking into why this wouldn't cause an issues by trying to track down where the pfnDataSent callback comes into play. I haven't tracked down anywhere in the CDC application where that is being checked. That got me to re-read the comments in question and I think we all jumped to a wrong conclusion.

    Going over the comments again:

    //! This function handles sending data to the host when a custom command is
    //! issued or non-standard descriptor has been requested on endpoint zero.  If
    //! the application needs notification when this is complete,
    //! <tt>psCallbacks->pfnDataSent</tt> in the tDeviceInfo structure must
    //! contain a valid function pointer.  This callback could be used to free up
    //! the buffer passed into this function in the \e pui8Data parameter.  The
    //! contents of the \e pui8Data buffer must remain unchanged until the
    //! <tt>pfnDataSent</tt> callback is received.

    The statement which starts with "If the application needs notification when this is complete, ..." I believe is the key here. That is what introduces the pfnDataSent callback. I think that comment is a precursor to the comment that the contents of the buffer must remain unchanged until the pfnDataSent callback is received. That condition only applies when the application needs a notification. But based on what I see, the application doesn't.

    Furthermore, digging deeper into the USBDCDSendDataEP0 itself, the USBDEP0StateTx call made at the end of it before it returns does check for the pfnDataSent callback anyways, so it is being checked just not at an application level. That would explain why I can't find any traces of the pfnDataSent being checked beyond inside of USBDEP0StateTx.

    So I think based on all of this, there isn't a bug... just some somewhat poor English (which isn't a first time occurrence with library software comments).

    Please let me know your thoughts on this.

    Best Regards,

    Ralph Jacobi