fix(cmsis_rtthread): 防止事件标志设置在未保护状态下更改#12
fix(cmsis_rtthread): 防止事件标志设置在未保护状态下更改#12wdfk-prog wants to merge 1 commit intoRT-Thread-packages:masterfrom
Conversation
- 事件标志设置由rtt函数内部保护后设置
WalkthroughModified an event flag operation in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmsis_rtthread.c`:
- 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.
| } | ||
|
|
||
| set_flags = event_cb->event.set |= flags; | ||
| set_flags = event_cb->event.set | flags; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the cmsis_rtthread.c file
find . -name "cmsis_rtthread.c" -type fRepository: 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(invalidef_idor ifflagshas 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 -40Repository: 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 3Repository: 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 -40Repository: 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 includeRepository: 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 -20Repository: 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->setand 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 thesetparameter, 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.
| 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.
1 participant
Summary by CodeRabbit