fix(cdc): stop arming OUT endpoint with a NULL buffer - #3058
Open
KentLee86 wants to merge 2 commits into
Open
Conversation
USBD_CDC_Receive() falls back to USBD_CDC_ClearBuffer() when the
receive queue has no room left for another 64 byte block. That helper
re-arms the OUT endpoint with a NULL application buffer:
USBD_LL_PrepareReceive(pdev, CDC_OUT_EP, 0, 0);
The endpoint is valid again while ep->xfer_buff is NULL. The next OUT
packet the host sends reaches HAL_PCD_IRQHandler(), which copies the
packet without checking the destination:
if (count != 0U)
{
USB_ReadPMA(hpcd->Instance, ep->xfer_buff, ep->pmaadress, count);
}
The packet lands at address 0 and the write faults. On a STM32F103C8
this is an imprecise bus fault escalated to a hard fault (CFSR
IMPRECISERR, HFSR FORCED), and the board stops answering until reset.
Do not arm the endpoint when there is no room. An unarmed bulk OUT
endpoint NAKs, the host retries, and USBSerial::read() and the
readBytes() family already call CDC_resume_receive() after dequeuing,
so the endpoint is armed with a real block as soon as the sketch
drains. That is the flow control this transfer needs.
Fixes stm32duino#1399
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
USBD_CDC_Receive() was its only caller. The helper is not part of the ST USB device middleware, it is local to this core, and the only thing it does is arm the OUT endpoint with a NULL application buffer, which is what made a receive overrun fault the MCU. Remove it so it cannot be reused. This also drops a USE_USBD_COMPOSITE branch that would not compile: it reads a bare classId instead of pdev->classId.
There was a problem hiding this comment.
Pull request overview
This PR prevents USB CDC bulk OUT endpoint re-arming with a NULL Rx buffer when the CDC receive queue is full, avoiding a hard fault caused by low-level USB drivers copying incoming OUT data to address 0x0. Instead, when the queue is full, the OUT endpoint is left unarmed so it NAKs and the host retries until the sketch drains data and CDC_resume_receive() can arm reception with a valid buffer.
Changes:
- Remove the
USBD_CDC_ClearBuffer()function and its header declarations (it had no remaining callers). - Update
USBD_CDC_Receive()to always attemptCDC_resume_receive()and otherwise leave the OUT endpoint unarmed (NAK-based flow control), avoidingNULLbuffer arming.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libraries/USBDevice/src/cdc/usbd_cdc.c | Removes USBD_CDC_ClearBuffer() so the stack no longer provides an unsafe “arm with NULL buffer” API. |
| libraries/USBDevice/src/cdc/usbd_cdc_if.c | Stops re-arming OUT reception with a NULL buffer on queue-full; relies on NAK + retry until CDC_resume_receive() can arm a real block. |
| libraries/USBDevice/inc/usbd_cdc.h | Removes USBD_CDC_ClearBuffer() declarations to match the implementation removal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
USBD_CDC_Receive()re-arms the bulk OUT endpoint with aNULLapplicationbuffer when the CDC receive queue is full. The next OUT packet is then copied
to address 0 by the PCD interrupt handler and the MCU faults. This PR stops
arming the endpoint in that case and lets it NAK instead.
This PR fixes/implements the following bugs/features
Motivation
USBD_CDC_Receive()commits the received block and then tries to reserve thenext one:
https://github.com/stm32duino/Arduino_Core_STM32/blob/4a8b28a08a2c4ba2e1a76e15a1f0d6de1e39ff7f/libraries/USBDevice/src/cdc/usbd_cdc_if.c#L241-L247
CDC_resume_receive()returnsfalsewhenCDC_ReceiveQueue_ReserveBlock()cannot reserve another 64 byte slot, and the fallback
USBD_CDC_ClearBuffer()arms the endpoint with a null buffer and a null length:
https://github.com/stm32duino/Arduino_Core_STM32/blob/4a8b28a08a2c4ba2e1a76e15a1f0d6de1e39ff7f/libraries/USBDevice/src/cdc/usbd_cdc.c#L1022-L1043
The endpoint is valid again while
ep->xfer_buffisNULL. When the hostsends the next packet, the "OUT Single Buffering" branch of the PCD interrupt
handler copies it without checking the destination — only
countis checked:https://github.com/stm32duino/Arduino_Core_STM32/blob/4a8b28a08a2c4ba2e1a76e15a1f0d6de1e39ff7f/system/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_hal_pcd.c#L2307-L2317
(The EP0 branch a few lines above does guard it, with
(ep->xfer_count != 0U) && (ep->xfer_buff != 0U).)So the packet is written to address 0.
An unarmed bulk OUT endpoint NAKs and the host retries, which is the flow
control this situation calls for.
USBSerial::read(),readBytes(),readBytesUntil()andreadStringUntil()all callCDC_resume_receive()after dequeuing, so the endpoint is armed again with a real block as soon as
the sketch drains anything. Nothing else needs to change.
The second commit removes
USBD_CDC_ClearBuffer(), which had no other caller.It is local to this core (the vendored ST middleware copy in
system/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.chasno such function), and arming an endpoint with a null buffer is not something
that is safe to call. Its
USE_USBD_COMPOSITEbranch also would not compile —it reads a bare
classIdinstead ofpdev->classId. Happy to drop thatcommit if you would rather keep the function around.
Validation
Board: STM32F103C8 "Blue Pill", USB CDC on PA11/PA12.
Reproducer — nothing but a drain loop:
Open the CDC port on the host and do a single
write(b"a" * 512). The boardstops printing
alive, stops answering on USART1 as well, and stays deaduntil reset. The same 512 bytes written in 16 byte chunks 20 ms apart are
handled fine, because the sketch drains in between — it is bytes in flight,
not length or content.
Halting the core over SWD right after the crash:
Measured on core 2.10.1 (PlatformIO
framework-arduinoststm32 4.21001.250617).stm32f1xx_hal_pcd.cis byte identical between 2.10.1 and currentmain, andthe two CDC hunks above are unchanged too, so
mainis affected the same way.The runtime behaviour of this patch was verified on that hardware by making
the
USBD_CDC_ClearBuffer()call a no-op with-Wl,--wrap=USBD_CDC_ClearBuffer, which leaves exactly the code path this PRleaves. At the stock queue size the sketch above then handles a single 16 KB
host write correctly, as does the larger application the bug was found in.
I have not re-run the hardware test with this source patch built from
main.Raising
CDC_RECEIVE_QUEUE_BUFFER_PACKET_NUMBER— the workaround suggested in#1399 — only moves the ceiling. In the larger application the fault moved from
130 bytes to about 1536 bytes; it does not remove it.
Build check for this patch: PlatformIO
bluepill_f103c8,framework = arduinopointed at this branch, sketch above, builds clean.
Scope
The fix is in
libraries/USBDevice, so it applies to every family, but I haveonly measured F1.
For what it is worth, the missing
xfer_buffcheck is not specific to F1. Insystem/Driversonmain, the "OUT Single Buffering" branch guards onlycount != 0Uin all 17 PMA based drivers (C0, C5, F0, F1, F3, G0, G4, H5, L0,L1, L4, L5, U0, U3, U5, WB, WBA), and the OTG
RXFLVLpath guards onlyBCNT != 0Uin F2, F4, F7 and H7. That is vendored ST code, so I have nottouched it here — with this PR the core no longer hands those drivers a null
buffer.
Code formatting
Checked locally with
CI/astyle/.astylerc(astyle 3.6.18 here, CI pins 3.1):no reformatting on any of the three files, and none on the unpatched
mainversions either, so the astyle version difference is not hiding anything.
Closing issues
Fixes #1399