Add bounds checking for arduino_pins array access in digital_pin_gpios path#6
Conversation
…rray Co-authored-by: soburi <458281+soburi@users.noreply.github.com>
Co-authored-by: soburi <458281+soburi@users.noreply.github.com>
Co-authored-by: soburi <458281+soburi@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds bounds validation for digital_pin_gpios-backed pin mapping to prevent out-of-bounds access of arduino_pins[] in the Zephyr Arduino core.
Changes:
- Add bounds checks to
local_gpio_port()/local_gpio_pin()fordigital_pin_gpiosbuilds. - Add early return in
global_gpio_pin_configure()to avoid out-of-rangearduino_pins[]indexing. - Ignore CodeQL-generated build/source-root artifacts in
.gitignore.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cores/arduino/zephyrCommon.cpp | Adds bounds checks for arduino_pins[] access in the digital_pin_gpios path. |
| .gitignore | Ignores CodeQL artifact directories/files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inline int global_gpio_pin_configure(pin_size_t pinNumber, int flags) { | ||
| #if DT_NODE_HAS_PROP(DT_PATH(zephyr_user), digital_pin_gpios) | ||
| if (pinNumber >= ARRAY_SIZE(arduino_pins)) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
In the digital_pin_gpios path, global_gpio_pin_configure() returns -1 for an invalid pinNumber. Elsewhere in this file invalid arguments typically return -EINVAL/-ENOTSUP (e.g. analogRead()), and Zephyr GPIO APIs return negative errno codes. Consider returning -EINVAL here as well to keep error semantics consistent (even if most callers currently ignore the return value).
| constexpr inline const struct device *local_gpio_port(pin_size_t gpin) { | ||
| #if DT_NODE_HAS_PROP(DT_PATH(zephyr_user), digital_pin_gpios) | ||
| return arduino_pins[gpin].port; | ||
| return (gpin < ARRAY_SIZE(arduino_pins)) ? arduino_pins[gpin].port : nullptr; |
There was a problem hiding this comment.
local_gpio_port() now returns nullptr and local_gpio_pin() can return pin_size_t(-1) for out-of-range pins, but at least one call site (e.g. attachInterrupt()) assumes a valid port/pin: it asserts pcb != nullptr even when port is nullptr, and then does BIT(local_gpio_pin(pinNumber)) which becomes a shift by -1. Please add consistent guards in callers (or in a shared helper) to early-return when local_gpio_port() is null and/or local_gpio_pin() is -1 before any indexing/bit-shifts/dereferences.
Addresses feedback from PR #4 regarding missing bounds validation when accessing
arduino_pins[gpin]in thedigital_pin_gpiosconfiguration path. Without bounds checking, invalid pin numbers cause out-of-bounds memory access across multiple call sites (digitalWrite/Read, interrupts, tone, etc.).Changes
local_gpio_port(): Returnnullptrwhengpin >= ARRAY_SIZE(arduino_pins)local_gpio_pin(): Returnpin_size_t(-1)whengpin >= ARRAY_SIZE(arduino_pins)global_gpio_pin_configure(): Early return-1before array access whenpinNumber >= ARRAY_SIZE(arduino_pins)The non-digital_pin_gpios path already had implicit bounds checking through
local_gpio_port_r()recursion.find_gpio_port_callback()null checking was added in commit c38fdbb.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.