RT-Thread / RT-Thread/rt-thread

[Bug] pin_api_attach_irq legacy path publishes hdr and args without IRQ protection

Open
#11,784 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug Component component: drivers
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:

  1. Application calls pin_api_attach_irq() to update the pin interrupt handler
  2. The function writes legacy_isr->hdr = new_handler
  3. Before writing legacy_isr->args = new_args, a pin interrupt fires
  4. pin_pic_handle_isr() executes and calls new_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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.