RT-Thread / RT-Thread/rt-thread
[Bug] pin_api_attach_irq legacy path publishes hdr and args without IRQ protection
Nobody has claimed this yet.
- Dominant language
- C
- Stars
- 12.2k
- Forks
- 5.4k
- Avg merge
- 4d 12h
- Merged PRs (30d)
- 40
Description
RT-Thread Version
v5.3.0 (master branch, commit 54e5164064dd7bf2e3f008109d173c36e45f8f69)
Affected area
Device drivers
Hardware/BSP vendor
Not applicable / Other
Architecture
Not applicable / Other
Board and hardware details
This issue affects any board where the GPIO controller does not implement the pin_attach_irq operation, forcing the code to fall back to the legacy ISR handler path.
Develop Toolchain
Other
Describe the bug
In components/drivers/pin/dev_pin_dm.c, when gpio->ops->pin_attach_irq is NULL (legacy fallback path), the function pin_api_attach_irq() writes the handler pair without masking IRQs:
legacy_isr->hdr = hdr;
legacy_isr->args = args;
Meanwhile, pin_pic_handle_isr() reads these fields from ISR context:
if (legacy_isr->hdr)
{
legacy_isr->hdr(legacy_isr->args);
}
Steps to reproduce the behavior
Race condition scenario:
- Application calls
pin_api_attach_irq()to update the pin interrupt handler - The function writes
legacy_isr->hdr = new_handler - Before writing
legacy_isr->args = new_args, a pin interrupt fires pin_pic_handle_isr()executes and callsnew_handler(old_args)with mismatched arguments
This can lead to:
- Incorrect argument being passed to the new handler
- Potential crashes if the new handler expects a different argument type/structure
- Unpredictable behavior
Expected behavior
The handler function pointer and its argument should be updated atomically. Either both should reflect the old values, or both should reflect the new values. No intermediate state should be visible to the ISR.
Other additional context
Affected code path: This bug only applies to the legacy fallback path when the GPIO controller does not implement the pin_attach_irq operation. Modern controllers that provide this operation are not affected.
Root cause: The two stores (hdr and args) are not atomic. On any architecture, an interrupt can occur between them.
Suggested fix: Protect the critical section with IRQ masking:
rt_base_t level = rt_hw_interrupt_disable();
legacy_isr->hdr = hdr;
legacy_isr->args = args;
rt_hw_interrupt_enable(level);
Or use the pin lock with rt_spin_lock_irqsave if appropriate.
Note: The volatile qualifier does not make the pair of stores atomic against interrupts.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in components/drivers/pin/dev_pin_dm.c and read pin_api_attach_irq() alongside pin_pic_handle_isr(), focusing on the legacy path used when pin_attach_irq is unavailable. Trace the handler and argument update sequence, then verify that an interrupt cannot observe a mixed pair and that both legacy and modern paths remain functional.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot, operating-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100