fix(dma): guard dmaDescriptors[] indexing on F4/F7 against invalid identifiers

[nerdCopter]

AI Generated [pull-request]

Summary

• Ports dmaIdentifierIsValid() from PR fix(h7): bounds-validate dmaIdentifier_e before dmaDescriptors[] indexing #1325 (H7 fix for issue fix(h7): dma_stm32h7xx.c — bounds-validate dmaIdentifier_e before indexing dmaDescriptors[] #1205) to dma_stm32f4xx.c and dma_stm32f7xx.c
• dmaGetOwner, dmaGetResourceIndex, dmaGetRefByIdentifier, dmaGetDescriptorByIdentifier, dmaSetHandler, and dmaInit indexed dmaDescriptors[] via DMA_IDENTIFIER_TO_INDEX() with no bounds check; dmaAllocate/dmaEnable only checked identifier != DMA_NONE, not the upper bound
• identifier == DMA_NONE underflows the index, identifier > DMA_LAST_HANDLER overflows it — undefined behavior/potential hardfault on any caller that passes an invalid identifier
• Currently latent: no known caller passes an out-of-range identifier to these functions on F4/F7 today
• BF 4.5-maintenance has zero bounds validation on the equivalent path (verified directly for the H7 fix) — this is EF-original defensive hardening, not a BF port

Closes #1341.

Test plan

• make clean_test && make test — all suites pass, 0 failures
• make HELIOSPRING TUNERCF405 SKYSTARSF405AIO PYRODRONEF7 FOXEERF722V4 FOXEERF405 APEXF7 TMOTORF7 — 8 succeeded, 0 failed, no new warnings
• CodeRabbit (--base upstream/master): 0 findings
• Hardware verification — not required; this is a defensive-only guard on a currently-latent, never-triggered path (matches PR fix(h7): bounds-validate dmaIdentifier_e before dmaDescriptors[] indexing #1325's rationale); no behavior change on any valid identifier

Summary by CodeRabbit

• Bug Fixes
  • Improved DMA handling by validating incoming DMA identifiers before accessing internal descriptor tables.
  • Prevented invalid or stale identifiers from triggering unintended state changes or unsafe memory access.
  • Standardized safe fallback behavior for invalid identifiers: allocation fails, enable/init/set-handler become no-ops, and queries for owner, resource index, descriptors, and references return safe defaults.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a8fdee8-2e06-41bb-a44a-6d185264cdca

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

F4 and F7 DMA drivers add identifier validation before indexing dmaDescriptors[]. Invalid identifiers now produce safe return values or early returns across allocation, initialization, handler setup, ownership, resource, reference, and descriptor accessors.

Changes

DMA identifier hardening

Layer / File(s)	Summary
Guard DMA operations src/main/drivers/dma_stm32f4xx.c, src/main/drivers/dma_stm32f7xx.c	Adds dmaIdentifierIsValid() and validates identifiers before allocation, enabling, initialization, or handler setup.
Return safe accessor defaults src/main/drivers/dma_stm32f4xx.c, src/main/drivers/dma_stm32f7xx.c	Returns OWNER_FREE, 0, or NULL from DMA accessors when identifiers are invalid.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• fix(dma): bounds-validate dmaIdentifier_e before dmaDescriptors[] indexing in dma_stm32f4xx.c / dma_stm32f7xx.c #1333 — Proposes the same bounds-validation changes across both F4 and F7 DMA drivers.

Possibly related PRs

• emuflight/EmuFlight#1325 — Adds the corresponding dmaIdentifierIsValid() guards to the H7 DMA driver.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the F4/F7 DMA bounds-check fix and matches the changed files.
Description check	✅ Passed	The description includes a clear summary, linked issue closure, and a test plan, which is mostly complete for this template.
Linked Issues check	✅ Passed	The PR implements the requested guard pattern for all affected F4/F7 DMA accessors and prevents invalid descriptor indexing for #1341.
Out of Scope Changes check	✅ Passed	The change set stays focused on defensive DMA identifier validation in the two requested drivers with no obvious unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[nerdCopter]

@coderabbitai , analyze the changes in this PR -- are they safe for merge and do not introduce problems, regression, inefficiency?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== PR metadata and changed files ==='
gh pr view 1344 --repo emuflight/EmuFlight --json number,title,baseRefName,headRefName,commits,statusCheckRollup \
  --jq '{number,title,base:.baseRefName,head:.headRefName,commits:(.commits|length),checks:[.statusCheckRollup[]? | {name,conclusion,status}]}'
git diff --find-renames origin/"$(gh pr view 1344 --repo emuflight/EmuFlight --json baseRefName --jq .baseRefName)"...HEAD -- \
  src/main/drivers/dma_stm32f4xx.c src/main/drivers/dma_stm32f7xx.c

echo '=== Driver outlines ==='
ast-grep outline src/main/drivers/dma_stm32f4xx.c --items all
ast-grep outline src/main/drivers/dma_stm32f7xx.c --items all
fd -a '^dma_stm32h7xx\.c$|^dma\.h$' src/main

echo '=== Identifier definitions and descriptor declarations ==='
rg -n -C 5 'DMA_NONE|DMA_LAST_HANDLER|dmaIdentifier_e|dmaDescriptors' src/main/drivers src/main | head -n 360

echo '=== All descriptor indexing and affected API call sites ==='
rg -n -C 3 'dmaDescriptors\s*\[|dma(SetHandler|GetOwner|GetResourceIndex|GetRefByIdentifier|GetDescriptorByIdentifier|Allocate|Enable|Init)\s*\(' \
  src/main --glob '*.{c,h}'

Length of output: 88865

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Direct index macro uses outside DMA drivers ==='
rg -n -C 4 'DMA_IDENTIFIER_TO_INDEX\s*\(' src/main --glob '*.{c,h}' \
  -g '!src/main/drivers/dma_stm32f4xx.c' \
  -g '!src/main/drivers/dma_stm32f7xx.c' \
  -g '!src/main/drivers/dma_stm32h7xx.c' \
  -g '!src/main/drivers/dma.c' \
  -g '!src/main/drivers/dma.h' || true

echo '=== Identifier getter / descriptor getter consumers (with enclosing context) ==='
rg -n -C 10 '\bdmaGet(RefByIdentifier|DescriptorByIdentifier|Owner|ResourceIndex)\s*\(' \
  src/main --glob '*.{c,h}'

echo '=== Relevant F4/F7 consumer configuration paths ==='
for f in \
  src/main/drivers/bus_spi.c \
  src/main/drivers/sdcard.c \
  src/main/drivers/sdcard_sdio_baremetal.c \
  src/main/drivers/serial_uart_stm32f4xx.c \
  src/main/drivers/serial_uart_stm32f7xx.c \
  src/main/drivers/adc_stm32f4xx.c \
  src/main/drivers/adc_stm32f7xx.c \
  src/main/drivers/pwm_output_dshot.c \
  src/main/drivers/pwm_output_dshot_hal.c \
  src/main/drivers/light_ws2811strip_stdperiph.c \
  src/main/drivers/light_ws2811strip_hal.c \
  src/main/drivers/transponder_ir_io_stdperiph.c \
  src/main/drivers/transponder_ir_io_hal.c; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    rg -n -C 12 '(dmaGet(RefByIdentifier|DescriptorByIdentifier)|dmaInit|dmaSetHandler|dmaEnable|dmaAllocate)' "$f" || true
  fi
done

echo '=== PR test changes and status update ==='
git diff --check origin/master...HEAD
git diff --stat origin/master...HEAD
git diff --name-only origin/master...HEAD | rg -n '(test|unit|dma)' || true
gh pr view 1344 --repo emuflight/EmuFlight --json statusCheckRollup \
  --jq '[.statusCheckRollup[]? | {name,conclusion,status}]'

Length of output: 48309

---

@nerdCopter No blocking issue found. The change looks safe and appropriate to merge once the in-progress target build matrix passes.

• Correct bounds: F4/F7 IDs are contiguous from 1 through DMA_LAST_HANDLER; dmaDescriptors contains DMA_LAST_HANDLER entries; and DMA_IDENTIFIER_TO_INDEX(id) is id - 1. The new predicate therefore permits exactly indices 0..DMA_LAST_HANDLER - 1, rejecting both DMA_NONE underflow and out-of-range overflow.
• Coverage: The guards are present before every changed dynamic dmaDescriptors[] access in the F4/F7 accessors and setup paths. No external direct uses of DMA_IDENTIFIER_TO_INDEX() bypass these driver APIs were found.
• Behavior: Valid IDs retain their existing behavior. Invalid IDs now return safe sentinel values or no-op rather than indexing invalid memory. This matches the already-established STM32H7 implementation.
• Callers: SPI’s descriptor lookup follows successful dmaAllocate(), which now guarantees the ID is valid. The SD-card invalid-configuration case now produces NULL/no-op instead of existing undefined behavior; that is an improvement, not a regression.
• Efficiency: The helper is static inline; it adds two inexpensive comparisons only in DMA setup/accessor calls, not in the IRQ handler hot path.
• Validation status: Existing unit tests, SITL, and static analysis are green. The target build jobs are still running, so I would wait for those before merging.

One minor follow-up opportunity—not merge-blocking—is to add focused tests for DMA_NONE and DMA_LAST_HANDLER + 1 return behavior if this driver layer has practical unit-test coverage.

[nerdCopter]

@coderabbitai, analyze the dma_bounds_unittest.cc added in commit e81649a. It covers the DMA_NONE / DMA_LAST_HANDLER + 1 boundary case you flagged as a non-blocking follow-up. Note it tests dma.h's enum contract (DMA_NONE, DMA_LAST_HANDLER, DMA_IDENTIFIER_TO_INDEX) with STM32F4 defined, not a direct call into dmaIdentifierIsValid() itself, since that function is a private static inline duplicated per MCU family in each .c file and not exposed via any shared header. Is this adequate coverage of the behavior you asked for, or is there a way to reach the real function without a disproportionate HAL/CMSIS stubbing effort?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Commit and changed files ==='
git show --stat --oneline e81649adc
git show --format=fuller --no-ext-diff --find-renames e81649adc -- \
  ':!src/main/drivers/dma_stm32f4xx.c' ':!src/main/drivers/dma_stm32f7xx.c'

echo '=== Locate and map the boundary test ==='
fd -a '^dma_bounds_unittest\.cc$' .
test_file="$(fd -a '^dma_bounds_unittest\.cc$' . | head -n 1 || true)"
if [ -n "$test_file" ]; then
  ast-grep outline "$test_file" --items all
  cat -n "$test_file"
fi

echo '=== F4/F7 helper implementations and all descriptor accesses ==='
for driver in src/main/drivers/dma_stm32f4xx.c src/main/drivers/dma_stm32f7xx.c; do
  if [ -f "$driver" ]; then
    echo "--- $driver ---"
    ast-grep outline "$driver" --items all --match 'dmaIdentifierIsValid|dmaAllocate|dmaEnable|dmaInit|dmaSetHandler|dmaGetOwner|dmaGetResourceIndex|dmaGetRefByIdentifier|dmaGetDescriptorByIdentifier'
    rg -n -C 7 'dmaIdentifierIsValid|dmaDescriptors\s*\[' "$driver"
  fi
done

echo '=== Enum/macro contract and test registration references ==='
rg -n -C 5 'DMA_NONE|DMA_LAST_HANDLER|DMA_IDENTIFIER_TO_INDEX' src/main test . \
  -g '*.{h,c,cc,cpp,CMakeLists.txt,cmake,make,mk}' 2>/dev/null | head -n 500
rg -n -C 4 'dma_bounds_unittest|dma_bounds' . \
  -g '*.{CMakeLists.txt,cmake,make,mk,cc,cpp,h}' 2>/dev/null || true

Length of output: 54040

---

@nerdCopter: Yes—this is adequate for the non-blocking follow-up I suggested, with one important qualification: it is a contract-level test, not a direct test of the production driver paths.

The test correctly establishes the F4/F7/H7 enum boundary invariant:

• DMA_NONE (0) is invalid.
• DMA_NONE + 1 is valid.
• DMA_LAST_HANDLER is valid.
• DMA_LAST_HANDLER + 1 is invalid.

That matches the exact predicate currently present in both production implementations:

• src/main/drivers/dma_stm32f4xx.c, Lines 79–81
• src/main/drivers/dma_stm32f7xx.c, Lines 89–91

It also compiles dma.h through the STM32F4 branch, so it validates the relevant stream-based dmaIdentifier_e definition rather than an unrelated host configuration.

The limitation is that dma_bounds_unittest.cc reimplements the predicate locally. Therefore, if a later edit changes the predicate in only F4 or F7—or removes a guard from an accessor—the test would still pass. It does not exercise the observable safety behavior such as:

• dmaAllocate(DMA_NONE, ...) == false
• dmaGetRefByIdentifier(DMA_LAST_HANDLER + 1) == NULL
• dmaGetOwner(...) == OWNER_FREE
• no-op behavior in dmaEnable, dmaInit, and dmaSetHandler.

However, I do not think expanding this PR to test those production functions is worthwhile. Reaching them directly means compiling the F4/F7 driver translation units and supplying sufficient RCC/NVIC/HAL/CMSIS symbols, register objects, and DMA descriptor dependencies. That is substantial scaffolding for code whose relevant logic is two comparisons, and it would be more fragile than the behavior being tested.

The cleaner alternatives would be either:

1. move the predicate into a shared header helper and test it directly, or
2. add a test-only hook / include-the-.c-file harness.

Both add coupling or production/test-surface complexity without meaningful additional value here.

So: the new test resolves my suggested follow-up adequately. The source review already confirms every newly protected F4/F7 dmaDescriptors[] access invokes the same predicate before indexing; this unit test usefully locks down the enum boundary assumptions behind that predicate. No merge blocker. 🐇

---

✏️ Learnings added

Learnt from: nerdCopter
URL: https://github.com/emuflight/EmuFlight/pull/1344

Timestamp: 2026-07-29T13:40:52.419Z
Learning: For STM32F4, STM32F7, and STM32H7 DMA drivers, `dmaIdentifierIsValid()` is intentionally a private `static inline` predicate duplicated in each MCU-family `.c` file rather than exposed by `src/main/drivers/dma.h`. The host unit test `src/test/unit/dma_bounds_unittest.cc` therefore tests the `dma.h` enum boundary contract (`DMA_NONE`, `DMA_LAST_HANDLER`, and `DMA_IDENTIFIER_TO_INDEX`) under the `STM32F4` define rather than linking the driver implementations, avoiding disproportionate HAL/CMSIS stubbing.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.