Fix SetStatusDelimiter: stack over-read and pointer-sign warnings
Two remaining issues in YEEP144_BluetoothUART.c, neither addressed by 1.2. SetStatusDelimiter() transmits sizeof(rxBuf) = 10 bytes from an 8-byte stack buffer (rxBuf is a different buffer declared earlier in the same function), so two bytes of stack garbage follow every "SO" command the module receives. Send strlen(txBuf) instead. The same function passes char* to HAL_UART_Transmit(), which takes const uint8_t*, producing three -Wpointer-sign warnings. They sit on the same lines as the over-read, which is part of why it went unnoticed; with the casts the file compiles warning-free. See CODE_FIXES.md for the full write-up.
This commit is contained in:
108
CODE_FIXES.md
Normal file
108
CODE_FIXES.md
Normal file
@@ -0,0 +1,108 @@
|
||||
# CODE_FIXES — YEEP-144 library
|
||||
|
||||
Two fixes to `YEEP144lib/src/YEEP144_BluetoothUART.c`, against the **1.2 release**.
|
||||
|
||||
Both are pre-existing issues that 1.2 does not address.
|
||||
|
||||
> **Already fixed in 1.2, not part of this change:** an earlier report covered
|
||||
> `YEEP144_BluetoothUART_Init()` entering the RN4678's command mode and
|
||||
> rebooting the module on every boot (by calling `SetStatusDelimiter()`
|
||||
> unconditionally). 1.2 has removed that call from `Init()`, so no change is
|
||||
> needed here. See "Suggested follow-up" below.
|
||||
|
||||
| # | Fix | Severity |
|
||||
|---|---|---|
|
||||
| 1 | `SetStatusDelimiter()` over-reads a stack buffer | Medium — undefined behaviour, and it sends garbage to the module |
|
||||
| 2 | Three `-Wpointer-sign` warnings in the same function | Low — but they sit on the same lines as #1 |
|
||||
|
||||
---
|
||||
|
||||
## Fix 1 — SetStatusDelimiter over-reads a stack buffer
|
||||
|
||||
### Root cause
|
||||
|
||||
```c
|
||||
uint8_t rxBuf[10];
|
||||
...
|
||||
char txBuf[8] = {0};
|
||||
if(prefix == '\0' && postfix == '\0'){
|
||||
sprintf(txBuf, "SO,,%c",'\r');
|
||||
}
|
||||
...
|
||||
HAL_UART_Transmit(&huart2, txBuf, sizeof(rxBuf), UART_POLL_TIMEOUT);
|
||||
^^^^^^^^^^^^ rxBuf is uint8_t[10]
|
||||
```
|
||||
|
||||
`rxBuf` is a **different, unrelated** buffer declared earlier in the same
|
||||
function. This transmits **10 bytes from an 8-byte array** — a two-byte stack
|
||||
over-read, so two bytes of stack garbage follow every `SO` command the module
|
||||
receives.
|
||||
|
||||
1.2 no longer calls this function from `Init()`, so it is only reachable if an
|
||||
application calls `YEEP144_BluetoothUART_SetStatusDelimiter()` directly. It is
|
||||
still a public API function, and the over-read is undefined behaviour either way.
|
||||
|
||||
### Patch
|
||||
|
||||
```c
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)txBuf, (uint16_t)strlen(txBuf),
|
||||
UART_POLL_TIMEOUT);
|
||||
```
|
||||
|
||||
`strlen()` is the right length: it stops at the terminating NUL that
|
||||
`sprintf()` wrote. `sizeof(txBuf)` would also fix the over-read, but would
|
||||
still transmit the padding NULs that `sprintf()` left in the buffer.
|
||||
|
||||
---
|
||||
|
||||
## Fix 2 — Three pointer-sign warnings
|
||||
|
||||
### Symptom
|
||||
|
||||
```
|
||||
warning: pointer targets in passing argument 2 of 'HAL_UART_Transmit' differ in
|
||||
signedness [-Wpointer-sign]
|
||||
```
|
||||
|
||||
at three call sites in `SetStatusDelimiter()`.
|
||||
|
||||
### Root cause
|
||||
|
||||
`HAL_UART_Transmit()` takes `const uint8_t *`; the function passes `char *`
|
||||
(two string literals, and `txBuf`).
|
||||
|
||||
### Patch
|
||||
|
||||
Cast at the three call sites:
|
||||
|
||||
```c
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)"$$$", 3, UART_POLL_TIMEOUT);
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)txBuf, (uint16_t)strlen(txBuf), UART_POLL_TIMEOUT);
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)"R,1", 3, UART_POLL_TIMEOUT);
|
||||
```
|
||||
|
||||
With this, the file compiles warning-free.
|
||||
|
||||
---
|
||||
|
||||
## Suggested follow-up (not part of this change)
|
||||
|
||||
1.2 removed the module configuration from `Init()`. The capability itself is
|
||||
still worth having, as an explicitly-named function rather than a side effect of
|
||||
startup — e.g. `YEEP144_BluetoothUART_ConfigureModule()`, documented as taking
|
||||
several seconds, entering the module's command mode, rebooting the module, and
|
||||
therefore dropping any active connection.
|
||||
|
||||
---
|
||||
|
||||
## Files changed
|
||||
|
||||
| File | Change |
|
||||
|---|---|
|
||||
| `YEEP144lib/src/YEEP144_BluetoothUART.c` | Both fixes — 7 insertions, 3 deletions against `main` |
|
||||
|
||||
## Repository note
|
||||
|
||||
`Drivers/` (the vendored ST HAL and CMSIS) is not in this repository: it is
|
||||
173 MB, of which 108 MB is ST's `.chm` user manuals. Fetch it from
|
||||
STMicroelectronics/STM32CubeH5 if you need to build.
|
||||
@@ -192,7 +192,7 @@ void YEEP144_BluetoothUART_SetStatusDelimiter(char prefix, char postfix){
|
||||
uint16_t rxSize;
|
||||
|
||||
// Enter command mode
|
||||
HAL_UART_Transmit(&huart2, "$$$", 3, UART_POLL_TIMEOUT);
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)"$$$", 3, UART_POLL_TIMEOUT);
|
||||
|
||||
// Wait to receive data back from BT module
|
||||
HAL_UARTEx_ReceiveToIdle(&huart2, rxBuf, 10, &rxSize, UART_POLL_TIMEOUT);
|
||||
@@ -211,13 +211,17 @@ void YEEP144_BluetoothUART_SetStatusDelimiter(char prefix, char postfix){
|
||||
else{
|
||||
sprintf(txBuf, "SO,%c,%c%c", prefix, postfix, '\r');
|
||||
}
|
||||
HAL_UART_Transmit(&huart2, txBuf, sizeof(rxBuf), UART_POLL_TIMEOUT);
|
||||
// rxBuf is a different, unrelated buffer (uint8_t rxBuf[10]); using its
|
||||
// size here transmits 10 bytes from this 8-byte array, so two bytes of
|
||||
// stack garbage follow every command. Send exactly the string.
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)txBuf, (uint16_t)strlen(txBuf),
|
||||
UART_POLL_TIMEOUT);
|
||||
|
||||
// Wait to receive data back from BT module
|
||||
HAL_UARTEx_ReceiveToIdle(&huart2, rxBuf, 10, &rxSize, UART_POLL_TIMEOUT);
|
||||
|
||||
// Reboot device to make changes
|
||||
HAL_UART_Transmit(&huart2, "R,1", 3, UART_POLL_TIMEOUT);
|
||||
HAL_UART_Transmit(&huart2, (const uint8_t *)"R,1", 3, UART_POLL_TIMEOUT);
|
||||
|
||||
// Wait to receive data back from BT module
|
||||
HAL_UARTEx_ReceiveToIdle(&huart2, rxBuf, 10, &rxSize, UART_POLL_TIMEOUT);
|
||||
|
||||
Reference in New Issue
Block a user