Repository navigation
dfs/posix: validate bounds and safely detach listener in signalfd_close - #11935
adilalperenciftci wants to merge 2 commits into
Conversation
|
|
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
🏷️ Tag: components_libcReviewers: @GorrayLi @mysterywolf Changed Files (Click to expand)
🏷️ Tag: components_lwpReviewers: @xu18838022837 Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-11-04 12:56 CST)
📝 Review Instructions
|
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
|
bcd55d6 to
0eb68b5
Compare
Rbb666
left a comment
Rbb666
left a comment
There was a problem hiding this comment.
Static code review of commit 0eb68b5.
The bounded slot lookup addresses the original out-of-bounds registration. Please address the following lifecycle and error-handling issues before merging. All locations below refer to components/libc/posix/io/signalfd/signalfd.c.
-
[P1] Protect registered LWP lifetimes (lines 77-81). A parent can create a signalfd, fork, and have the child register through read/poll before exiting and being reaped. The inherited descriptor keeps the same dfs_file alive, so the child's non-final close does not invoke signalfd_close. Registration does not acquire an LWP reference or arrange exit-time deregistration, leaving sfd->lwp[] pointing at the freed child. The parent's final close then dereferences that pointer.
Suggested change: unregister each process's listener and clear its slot during process exit, with the necessary lifetime protection until detachment completes. Final close must not traverse unprotected raw LWP pointers.
-
[P1] Synchronize listener removal with notification traversal (lines 87-88). lwp_signal_notify() traverses this list under LWP_LOCK, whereas removal here only holds sfd->lock. Once signalfd_callback() releases sfd->lock, a closing thread can unlink and free the current listener before the signal sender resumes the iterator's node = node->next step. That step then reads freed memory, even if the callback has already completed.
Suggested change: use shared synchronization for registration, traversal, and removal, and ensure in-flight notifications have finished before freeing listeners/context. Do not simply acquire LWP_LOCK while holding sfd->lock, because the signal path already takes those locks in the opposite order.
-
[P1] Finish vnode cleanup before releasing the fd (lines 378-379). If allocating rt_lwp_notify fails, fd_release(fd) can destroy df through fdt_fd_release -> dfs_file_destroy. The newly added df->vnode access therefore dereferences an already freed df.
Suggested change: detach the initialized mutex and release the vnode/context while their owners are still valid, then release the fd and return immediately. The existing fall-through to sfd->sig_num and df->flags also accesses freed objects and needs to be terminated as part of this cleanup path.
-
[P2] Propagate registration-limit failures to read/poll (lines 295-299). Both signalfd_read() and signalfd_poll() ignore the new -EMFILE result. With RT_SIGNALFD_MAX_NUM=1, for example, a parent creates the fd and a forked child attempts a blocking read. Registration fails, but read still waits without a listener for that process; a masked target signal sent to the child cannot wake it through signalfd notification. Poll likewise proceeds without exposing the failure.
Suggested change: return the registration error from read, and report POLLERR from poll while handling any poll registration already added. Check signalfd_add_notify() before committing the caller to a wait.
This review is based on source inspection and call-path analysis; no build or runtime validation was performed.
Rbb666
left a comment
Rbb666
left a comment
There was a problem hiding this comment.
Static code review of commit 0eb68b5.
The bounded slot lookup addresses the original out-of-bounds registration. Please address the following lifecycle and error-handling issues before merging. All locations below refer to components/libc/posix/io/signalfd/signalfd.c.
-
[P1] Protect registered LWP lifetimes (lines 77-81). A parent can create a signalfd, fork, and have the child register through read/poll before exiting and being reaped. The inherited descriptor keeps the same dfs_file alive, so the child's non-final close does not invoke signalfd_close. Registration does not acquire an LWP reference or arrange exit-time deregistration, leaving sfd->lwp[] pointing at the freed child. The parent's final close then dereferences that pointer.
Suggested change: unregister each process's listener and clear its slot during process exit, with the necessary lifetime protection until detachment completes. Final close must not traverse unprotected raw LWP pointers.
-
[P1] Synchronize listener removal with notification traversal (lines 87-88). lwp_signal_notify() traverses this list under LWP_LOCK, whereas removal here only holds sfd->lock. Once signalfd_callback() releases sfd->lock, a closing thread can unlink and free the current listener before the signal sender resumes the iterator's node = node->next step. That step then reads freed memory, even if the callback has already completed.
Suggested change: use shared synchronization for registration, traversal, and removal, and ensure in-flight notifications have finished before freeing listeners/context. Do not simply acquire LWP_LOCK while holding sfd->lock, because the signal path already takes those locks in the opposite order.
-
[P1] Finish vnode cleanup before releasing the fd (lines 378-379). If allocating rt_lwp_notify fails, fd_release(fd) can destroy df through fdt_fd_release -> dfs_file_destroy. The newly added df->vnode access therefore dereferences an already freed df.
Suggested change: detach the initialized mutex and release the vnode/context while their owners are still valid, then release the fd and return immediately. The existing fall-through to sfd->sig_num and df->flags also accesses freed objects and needs to be terminated as part of this cleanup path.
-
[P2] Propagate registration-limit failures to read/poll (lines 295-299). Both signalfd_read() and signalfd_poll() ignore the new -EMFILE result. With RT_SIGNALFD_MAX_NUM=1, for example, a parent creates the fd and a forked child attempts a blocking read. Registration fails, but read still waits without a listener for that process; a masked target signal sent to the child cannot wake it through signalfd notification. Poll likewise proceeds without exposing the failure.
Suggested change: return the registration error from read, and report POLLERR from poll while handling any poll registration already added. Check signalfd_add_notify() before committing the caller to a wait.
This review is based on source inspection and call-path analysis; no build or runtime validation was performed.
github-actions
Bot
added
RT-Smart
Validate array bounds on signalfd notification registration and remove the unsafe global index counter. Tie the registered process lifetime to the registration: each slot in sfd->lwp[] holds an LWP reference, and the listener node carries a detach hook that the process exit path runs under LWP_LOCK to unlink the node, clear the slot and drop the reference. Final close therefore never walks a freed process, and a child that registered through an inherited descriptor is unregistered when it exits. Move every change of lwp->signalfd_notify_head behind lwp_signal_notify_* helpers in the lwp core, which take LWP_LOCK, the lock lwp_signal_notify() traverses the list under. The lock order is LWP_LOCK -> sfd->lock, the same order the signal path already uses; sfd->lock is never held while taking LWP_LOCK, and a node is only unlinked under LWP_LOCK, so no in-flight notification can be inside it when it is freed. Tear down the vnode, context and mutex before fd_release() when the initial registration fails, and return instead of falling through to freed objects. Propagate registration failures: read returns the error and poll reports POLLERR before anything is added to the poll queue. Fixes: RT-Thread#11933 Signed-off-by: Adil Alperen Ciftci <adilalperenciftci@users.noreply.github.com>
5255638 to
4b130fe
Compare
|
Pushed 4b130fe with all four items addressed, rebased on current master.
Verification: The failing "Static code analysis" job was not about this file: its diff against master picked up unrelated files ( |
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
Review / 审查
我把新增的提交对照 Rbb666 提的四点逐条看了一遍，生命周期模型现在是自洽的，几个关键不变量都能对上。
I went through the follow-up commit against the four points Rbb666 raised. The lifecycle model now holds together and the key invariants check out.
已确认的部分 / What looks right
- 注册时为
sfd->lwp[]槽位持有一个 LWP 引用，close 和_resr_cleanup()→lwp_signal_notify_detach()都是先清槽位、再减引用，所以“槽位非空 ⇒ lwp 指针有效”这个前提成立。signalfd.c#L330 · lwp_signal.c#L508 - 链表的所有修改都移到了
LWP_LOCK下，而signalfd_close()是先释放sfd->lock再调用lwp_signal_notify_remove_queue()，与signalfd_callback()的LWP_LOCK -> sfd->lock顺序一致，没有反向持锁。signalfd.c#L70 · lwp_signal.c#L481 signalfd_read()/signalfd_poll()在注册失败时都不会再进入队列/等待，POLLERR和错误返回的时机是对的。signalfd.c#L127signalfd_do()的失败路径先 detach mutex、释放 vnode/ctx，再fd_release()并直接返回，原来 fall-through 到已释放对象的问题已经消除。signalfd.c#L476
1. sfd 本身的生命周期 / Lifetime of the context itself
After lwp_signal_notify_add(), the code re-acquires sfd->lock to re-check the slot (signalfd.c#L409). Nothing pins the sfd context across that window: the slot keeps the lwp alive, not the file/vnode. read()/poll() do not take a file reference for the duration of the call (fd_get() only looks the file up), so another thread can close the last descriptor, run signalfd_close() and free both sfd and the vnode while this thread is between lwp_signal_notify_add() and the slot re-check.
This assumption already exists in signalfd_poll()/signalfd_read() themselves, so it is not a regression from this PR, but the new re-check adds one more post-unlock dereference. Please either state the assumption explicitly (concurrent close of the same fd is not supported) or hold a file/vnode reference across registration.
这个点不是本 PR 引入的回归，read/poll 原本就假设 sfd 在调用期间有效；但新加的“解锁后再查一次槽位”又多了一次 sfd 解引用。建议要么把“同一 fd 不允许并发 close”写成明确前提，要么在注册期间持有 file/vnode 引用。
2. Close 循环的加锁粒度 / Lock granularity in signalfd_close()
The loop takes and releases sfd->lock once per slot (signalfd.c#L92). Since it only reads and clears slots, it can hold the lock once, collect the non-NULL lwp pointers, release, and then do the per-lwp removal/drop work outside. Fewer lock round-trips and the "clear first, unlink later" invariant reads more directly. Optional.
3. SIGNALFD_SHART_MAX 拼写 / Typo
It is pre-existing, but this PR already reflows the macro block (signalfd.c#L14); renaming it to SIGNALFD_SHARE_MAX (or SIGNALFD_MAX_LWP) would be a cheap drive-by cleanup.
I don't see a correctness problem with the new locking; item 1 is the only one I would like clarified before merge.
State the lifetime assumption of the signalfd context explicitly: the descriptor the caller operates on pins the context for the whole read, poll and registration, and a concurrent close of that same descriptor by another thread is not supported, which is what the existing read and poll paths already rely on. The post-registration slot re-check therefore does not add a new requirement. Clear all registration slots in one pass under sfd->lock in close and run the per-process unlink and reference drop outside the lock afterwards, so the "clear first, unlink later" invariant is one block instead of a lock round-trip per slot. Rename SIGNALFD_SHART_MAX to SIGNALFD_SHARE_MAX while the macro block is being touched. Signed-off-by: Adil Alperen Ciftci <adilalperenciftci@users.noreply.github.com>
|
Thanks for the careful read. Follow-up in
No behavioural change beyond the lock round-trips; the invariant "slot non-null ⇒ lwp valid" and the LWP_LOCK → sfd->lock order are unchanged. |
Rbb666
left a comment
Rbb666
left a comment
There was a problem hiding this comment.
[P1] Fix the lock-order inversion between close and process exit — line 110
dfs_file_close() holds the global DFS file lock while calling signalfd_close(). The new lwp_signal_notify_remove_queue() call then takes the target process's LWP lock. The exit path takes these locks in reverse order: _resr_cleanup() holds LWP_LOCK(lwp) while __exit_files() calls dfs_fdtable_drop_fd(), which takes the DFS file lock.
One possible sequence:
- A child registers on an inherited signalfd, then closes its copy while the parent keeps the fd open.
- The parent's final close saves the child pointer in lwps[] and is preempted before removing the listener.
- The child exits, detaches its listener, then enters __exit_files() holding its LWP lock and waits for the DFS file lock.
- The parent resumes and waits for the child's LWP lock.
Neither can proceed. Clearing the slots first does not prevent this because close still uses the saved pointer. The close and exit paths need a consistent lock order.
[P2] Install the listener before making registration visible — lines 393–397
The slot is assigned and sfd->lock is released before lwp_signal_notify_add() links the listener at line 411. Another thread can find the slot and return success from signalfd_add_notify() while the listener is still missing.
After a fork, two threads in the child can poll the inherited fd. T1 assigns the slot and is preempted before linking the listener. T2 sees the slot, skips registration, and starts waiting. A blocked signal sent to the child at this point finds no listener, so it never reaches the signalfd queue. Linking the listener later does not copy pending signals into that queue, leaving the poll/read waiting for another signal.
Synchronize the lookup, listener insertion, and slot assignment so that an existing registration always has an installed listener.
[P2] Repeat the registration lookup after reacquiring the lock — lines 388–392
The original slot being empty does not guarantee that the process is still unregistered.
T1 can select slot i and pause. Another process takes i, then T2 registers T1's process in slot j. If the other process exits and frees i before T1 resumes, this condition is false. T1 skips the lookup and registers the same process again in i.
This creates two listeners for the same process and signalfd, causing duplicate signal notifications and consuming an extra registration slot.
Call signalfd_find_slot() unconditionally after reacquiring sfd->lock, and use the updated result before assigning a slot.
Summary
Enforce bounds checking on signalfd notification registrations and cleanly detach notification listeners during signalfd_close.
Technical Breakdown
Fixes: #11933