Skip to content

fix(cmsis_rtthread): 防止事件标志设置在未保护状态下更改 #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wdfk-prog wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(cmsis_rtthread): 防止事件标志设置在未保护状态下更改 #12

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmsis_rtthread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1367,7 +1367,7 @@ uint32_t osEventFlagsSet(osEventFlagsId_t ef_id, uint32_t flags)
return ((uint32_t)osFlagsErrorParameter);
}

set_flags = event_cb->event.set |= flags;
set_flags = event_cb->event.set | flags;
Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate and examine the cmsis_rtthread.c file
find . -name "cmsis_rtthread.c" -type f

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 92

🏁 Script executed:

# Once found, read the context around line 1370 to understand the osEventFlagsSet function
cat -n cmsis_rtthread.c | sed -n '1350,1390p'

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 1705

🌐 Web query:

CMSIS-RTOS2 osEventFlagsSet specification return value requirements

💡 Result:

Per the CMSIS‑RTOS2 specification, osEventFlagsSet(ef_id, flags) shall return:

  • On success: the event flags value after setting the requested bits (i.e., the updated flags stored in the object). [1]
  • On failure: an error code with the highest bit set (per “Flags Functions Error Codes”), e.g. osFlagsErrorParameter (invalid ef_id or if flags has the highest bit set), osFlagsErrorResource, osFlagsErrorUnknown, osFlagsErrorSafetyClass, etc. [1]

Implication: the input flags value must not set bit 31, because that bit is reserved to indicate an error return. [1]

Source: CMSIS‑RTOS2 Event Flags API documentation. [1]

References
[1] ARM CMSIS‑RTOS2 “Event Flags” (osEventFlagsSet) documentation, CMSIS v6: (arm-software.github.io)

Citations:

🏁 Script executed:

# Look at the complete osEventFlagsClear function to understand the pattern
cat -n cmsis_rtthread.c | sed -n '1384,1410p'

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 1012

🏁 Script executed:

# Search for rt_event_send definition or usage pattern to understand atomic guarantees
rg "rt_event_send|rt_hw_interrupt" cmsis_rtthread.c -A 2 -B 2 | head -40

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 752

🏁 Script executed:

# Check if there are other functions in the file that handle similar flag/event operations
rg "event\.set" cmsis_rtthread.c -B 3 -A 3

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 563

🏁 Script executed:

# Search for rt_event_send definition to understand what it does
rg "rt_event_send" . -t c -B 5 -A 10 | grep -A 15 "rt_event_send.*{" | head -40

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 56

🏁 Script executed:

# Also check if rt_event_send is from RT-Thread kernel by looking for includes
head -50 cmsis_rtthread.c | grep -i include

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 182

🏁 Script executed:

# Search for how other functions in the file handle rt_event operations
rg "rt_event" cmsis_rtthread.c | head -20

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 330

🏁 Script executed:

# Look for the complete context of how event flags are managed - check if there's a pattern of reading after operations
cat -n cmsis_rtthread.c | sed -n '1400,1425p'

Repository: RT-Thread-packages/CMSIS_RTOS2

Length of output: 1011

🌐 Web query:

RT-Thread rt_event_send behavior and atomicity - does it modify rt_event->set field

💡 Result:

Yes. rt_event_send(event, set) does modify the event object’s event->set field: it ORs in the bits you send (event->set |= set). While scanning the suspend list, if a waiting thread’s receive options include RT_EVENT_FLAG_CLEAR, the implementation may also clear the corresponding bits from event->set (event->set &= ~thread->event_set) when the condition is satisfied and the thread is resumed. [1]

Atomicity / concurrency behavior:

  • The update of event->set and the walk of the waiting-thread list are done inside a critical section created by disabling interrupts (rt_hw_interrupt_disable() … rt_hw_interrupt_enable()), so on a typical single-core RT-Thread system this makes the operation atomic w.r.t. ISRs and preempting threads. [1][2]
  • Semantically, RT-Thread’s docs describe rt_event_send() as “setting the event flag value of the event set object” according to the set parameter, then checking the wait list to wake matching threads—i.e., it is meant to mutate the event flags in the object. [3]

Implication: events are level-triggered / non-queued: sending the same bit multiple times before it’s cleared has the same effect as sending once (the bit is just already set). [4]

Sources
[1] RT-Thread rt_event_send implementation (shows event->set |= set under interrupt disable, and conditional clear) — https://www.cnblogs.com/kehuadong/p/13923485.html
[2] RT-Thread interrupt lock description (rt_hw_interrupt_disable/enable critical section semantics) — https://www.rt-thread.io/document/site/programming-manual/interrupt/interrupt/
[3] RT-Thread docs: rt_event_send() sets event flags then traverses suspend list to wake threads — https://rt-thread.github.io/rt-thread/page_thread_sync.html
[4] RT-Thread docs: events are not queuing (“sending the same event…effect is equivalent to sending only once”) — https://www.rt-thread.io/document/site/programming-manual/ipc1/ipc1/

Return value can be stale if flags change between read and send.

Per CMSIS-RTOS2 specification, osEventFlagsSet must return the event flags value after setting. The current implementation at line 1370 computes set_flags before calling rt_event_send(), which creates a race condition: another thread may modify flags between the read and the send operation, causing the returned value to not reflect the actual flags stored after the operation.

Since rt_event_send() performs event->set |= flags inside its own critical section but the pre-read at line 1370 is unprotected, the computed value can become stale. Read event_cb->event.set after rt_event_send() under a critical section (using rt_hw_interrupt_disable/enable()) to ensure the return value matches the actual stored flags—consistent with the pattern already used in osEventFlagsClear().

🔧 Suggested adjustment 
-    set_flags = event_cb->event.set | flags;
-
-    result = rt_event_send(&(event_cb->event), flags);
-
-    if (RT_EOK == result)
-        return set_flags;
-    else
-        return osFlagsError;
+    result = rt_event_send(&(event_cb->event), flags);
+
+    if (RT_EOK == result)
+    {
+        rt_ubase_t level = rt_hw_interrupt_disable();
+        set_flags = event_cb->event.set;
+        rt_hw_interrupt_enable(level);
+        return set_flags;
+    }
+    else
+        return osFlagsError;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
set_flags = event_cb->event.set | flags;
result = rt_event_send(&(event_cb->event), flags);
if (RT_EOK == result)
{
rt_ubase_t level = rt_hw_interrupt_disable();
set_flags = event_cb->event.set;
rt_hw_interrupt_enable(level);
return set_flags;
}
else
return osFlagsError;
🤖 Prompt for AI Agents 
In `@cmsis_rtthread.c` at line 1370, The current computation of set_flags before
calling rt_event_send() can return a stale value; modify osEventFlagsSet so you
call rt_event_send(event_cb->event, flags) first, then enter a critical section
using rt_hw_interrupt_disable() / rt_hw_interrupt_enable() and read
event_cb->event.set to compute the return value (as done in
osEventFlagsClear()), ensuring the returned flags reflect the actual stored
flags after the send; update references to set_flags accordingly.


result = rt_event_send(&(event_cb->event), flags);

Expand Down