USARTv1/v2: add osalDbgAssert for speed == 0 in usart_init#73
Open
vlordier wants to merge 1 commit into
Open
Conversation
In both USARTv1 and USARTv2 hal_serial_lld.c, usart_init() divides by config->speed to compute the BRR register value without first checking that speed is non-zero: USARTv1: brr = (sdp->clock + config->speed/2) / config->speed; USARTv2: brr = (clock + config->speed / 2) / config->speed; USARTv2 LPUART: brr = (clock * 256 + config->speed/2) / config->speed; Passing speed = 0 triggers integer division-by-zero, which is undefined behaviour in C. Depending on the CPU and toolchain, this may cause a hard fault, return garbage, or be silently optimised away, leaving the USART misconfigured. Both files already use osalDbgAssert() to validate the computed BRR range (e.g. "invalid BRR value"); this patch adds a matching check at the entry of the baud-rate calculation section, before any arithmetic on config->speed occurs. The fix is debug-only (osalDbgAssert), consistent with ChibiOS convention. A release build continues to exhibit undefined behaviour when speed = 0, but that is an existing driver contract; the assert gives developers an actionable trap during development.
There was a problem hiding this comment.
Pull request overview
Adds a debug-time precondition check to STM32 USART low-level serial drivers to prevent undefined behavior (division by zero) when SerialConfig.speed is accidentally set to 0.
Changes:
- Add
osalDbgAssert(config->speed != 0U, "invalid baud rate (zero)")at the start of baud-rate/BRR computation in USARTv1. - Add the same assert at the start of baud-rate/BRR computation in USARTv2 (including the LPUART path).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| os/hal/ports/STM32/LLD/USARTv2/hal_serial_lld.c | Adds debug assert guarding config->speed before BRR arithmetic to avoid divide-by-zero UB. |
| os/hal/ports/STM32/LLD/USARTv1/hal_serial_lld.c | Adds debug assert guarding config->speed before BRR arithmetic to avoid divide-by-zero UB. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Problem
In both
USARTv1/hal_serial_lld.candUSARTv2/hal_serial_lld.c, theusart_init()function divides byconfig->speedto compute the BRR register without first checking it is non-zero:Passing
speed = 0triggers integer division-by-zero, which is undefined behaviour in C (ISO C11 §6.5.5 ¶5). Depending on CPU and toolchain this may: cause a HardFault trap, return garbage leaving the USART misconfigured, or be silently optimised away.The USARTv2 LPUART path is additionally affected through the range check
clock >= config->speed * 3U: whenspeed = 0, the multiplication evaluates to 0 and the assert passes vacuously, not catching the error.This was identified while building Rust safety wrappers for ChibiOS peripherals and writing parity tests comparing C driver behaviour against the Rust implementation.
Fix
Add
osalDbgAssert(config->speed != 0U, "invalid baud rate (zero)")at the top of the baud-rate calculation section, before any arithmetic onconfig->speed. Placement and style are identical to the existing BRR range validation:Notes
osalDbgAssertcompiles away in release builds. A release build withspeed = 0continues to have undefined behaviour; this is consistent with how ChibiOS handles other parameter preconditions.hal_uart_lld.cis not included in this patch: the UART HAL driver does not useconfig->speeddirectly for BRR; baud rate comes from theSerialConfigpassed to the serial driver. A separate audit ofhal_uart_lld.cmay be warranted.