Part Number: TM4C123GH6PM
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 188.8.131.52.
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.
In reply to Charles Tsai:
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.
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?
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:
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.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.
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):
// sLineCoding is stored in the stack.
MAP_USBDevEndpointDataAck(psInst->ui32USBBase, USB_EP_0, false);
// Copy the linecoding data into the local struct
// Send the address of sLineCoding, which is a pointer to data on the stack
USBDCDSendDataEP0(0, (uint8_t *)&sLineCoding, sizeof(tLineCoding));
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.
Okay, I understand the concern you are raising fully now. I am looking into it further, stay tuned please.
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.
All content and materials on this site are provided "as is". TI and its respective suppliers and providers of content make no representations about the suitability of these materials for any purpose and disclaim all warranties and conditions with regard to these materials, including but not limited to all implied warranties and conditions of merchantability, fitness for a particular purpose, title and non-infringement of any third party intellectual property right. TI and its respective suppliers and providers of content make no representations about the suitability of these materials for any purpose and disclaim all warranties and conditions with respect to these materials. No license, either express or implied, by estoppel or otherwise, is granted by TI. Use of the information on this site may require a license from a third party, or a license from TI.
TI is a global semiconductor design and manufacturing company. Innovate with 100,000+ analog ICs andembedded processors, along with software, tools and the industry’s largest sales/support staff.