Skip to content

fix: monitor X11 lock state changes through XKB - #128

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
yixinshark:fix/x11-lock-state-notifications
Aug 6, 2026
Merged

fix: monitor X11 lock state changes through XKB#128
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
yixinshark:fix/x11-lock-state-notifications

Conversation

@yixinshark

@yixinshark yixinshark commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • subscribe to XKB modifier lock state notifications independently of passive shortcut grabs
  • propagate Caps Lock and Num Lock changes through the key handler
  • deduplicate the exported D-Bus state change signals

Tests

  • cmake --build build --target plugin-dde-shortcut -j2
  • shortcut unit tests (28 assertions passed)
  • git diff --check

Pms: BUG-372751

Summary by Sourcery

Handle X11 keyboard lock state changes via XKB and ensure consistent, deduplicated propagation of Caps Lock and Num Lock state updates through the shortcut subsystem.

Bug Fixes:

  • Monitor X11 Caps Lock and Num Lock state changes using XKB and propagate them through the key handling pipeline.
  • Deduplicate Num Lock and Caps Lock D-Bus state change emissions to avoid redundant signals.

Enhancements:

  • Expose lock state changes from the X11 key handler to the keybinding manager via new signals and slots.
  • Track last known Num Lock and Caps Lock states in the keybinding manager to prevent unnecessary updates.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @yixinshark, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Reviewer's Guide

This PR wires XKB-based monitoring of X11 modifier lock state (Caps/Num Lock) into the existing shortcut backend, propagates lock changes through the key handler and keybinding manager, and centralizes/filters emission of the exported D-Bus state-change signals to avoid duplicates.

Sequence diagram for XKB lock state propagation

sequenceDiagram
    participant XServer
    participant X11KeyHandler
    participant KeybindingManager
    participant DbusClients

    XServer->>X11KeyHandler: XkbStateNotify
    X11KeyHandler->>X11KeyHandler: notifyLockStateChange(event)
    alt [Num Lock changed]
        X11KeyHandler-->>KeybindingManager: numLockStateChanged(on)
        KeybindingManager->>KeybindingManager: updateNumLockState(on)
        KeybindingManager-->>DbusClients: NumLockStateChanged(state)
    end
    alt [Caps Lock changed]
        X11KeyHandler-->>KeybindingManager: capsLockStateChanged(on)
        KeybindingManager->>KeybindingManager: updateCapsLockState(on)
        KeybindingManager-->>DbusClients: CapsLockStateChanged(state)
    end
Loading

File-Level Changes

Change Details Files
Subscribe to XKB lock state notifications and handle them in the X11 key handler
  • Include XKB proto headers and store XKB event base opcode in the X11 key handler
  • Add enableLockStateMonitoring() to query the XKB extension, subscribe to XkbStateNotify with XkbModifierLockMask, and flush the display
  • Invoke enableLockStateMonitoring() during X11KeyHandler construction after modifier mask refresh
  • In the XCB event loop, route XKB events (matching m_xkbEventBase) to a new notifyLockStateChange() handler
  • Implement notifyLockStateChange() to filter state notifications by XkbStateNotify and XkbModifierLockMask and emit num/caps lock signals based on lockedMods and existing masks
src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.cpp
src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.h
Propagate lock state changes through the abstraction layer and deduplicate D-Bus state change signals
  • Extend AbstractKeyHandler with numLockStateChanged(bool) and capsLockStateChanged(bool) signals
  • Connect the key handler’s lock state signals to new slots in KeybindingManager that update internal last-state fields and only emit D-Bus signals on real changes
  • Initialize m_lastNumLockState and m_lastCapsLockState in the KeybindingManager constructor using existing getters
  • Refactor SetNumLockState and SetCapsLockState to delegate to the new update* slots instead of emitting signals directly, avoiding duplicate emission
src/plugin-qt/shortcut/src/backend/abstractkeyhandler.h
src/plugin-qt/shortcut/src/core/keybindingmanager.cpp
src/plugin-qt/shortcut/src/core/keybindingmanager.h

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@yixinshark
yixinshark force-pushed the fix/x11-lock-state-notifications branch from cc94572 to 0639b49 Compare August 5, 2026 08:03
Subscribe to XKB modifier lock state notifications independently of passive shortcut grabs.
Propagate Caps Lock and Num Lock changes through the key handler and deduplicate the exported D-Bus signals.
This also observes lock state changes made through XKB APIs without physical key press events.

独立于被动快捷键抓取订阅 XKB 修饰键锁定状态通知。
通过按键处理后端传递大小写锁定和数字锁定状态变化,并对导出的 D-Bus 信号去重。
同时支持检测通过 XKB API 直接修改且没有物理按键事件的锁定状态变化。

Log: monitor X11 lock state changes through XKB
Pms: BUG-372751
Change-Id: I923a352f91a63218d287bb013464538a65b27c56
@yixinshark
yixinshark force-pushed the fix/x11-lock-state-notifications branch from 0639b49 to 67fde5a Compare August 6, 2026 06:16
@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: robertkill, yixinshark

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yixinshark

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pr force merged! (status: blocked)

@deepin-bot
deepin-bot Bot merged commit eac2a2f into linuxdeepin:master Aug 6, 2026
7 of 8 checks passed
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

★ 总体评分:60分

■ 【总体评价】

代码实现了X11下物理锁键状态变更的底层监听与上层去重同步,但存在堆内存越界读取漏洞
逻辑基本正确但因内存越界读取安全漏洞及代码重复问题扣40分

■ 【详细分析】

  • 1.语法逻辑(存在错误)✕

X11KeyHandler::notifyLockStateChange 函数中,使用 std::memcpy(&stateEvent, event, sizeof(stateEvent)) 进行内存拷贝。event 的静态类型为 xcb_generic_event_t*,该结构体大小固定为32字节。而 xkbStateNotify 的大小通过 static_assert 验证等于 sz_xkbStateNotify(通常远大于32字节)。直接按 sizeof(stateEvent) 拷贝会读取超出 xcb_generic_event_t 结构体定义范围的内存,在C++严格语义下构成越界读取的未定义行为,尽管XCB底层实际分配了足够的连续内存。
潜在问题:越界读取导致未定义行为,可能引发程序崩溃或被静态分析工具误报
建议:使用 reinterpret_cast 进行指针转换,或确保拷贝长度不超过 xcb_generic_event_t 的安全范围

  • 2.代码质量(存在严重问题)✕

KeybindingManager 中新增的 updateNumLockStateupdateCapsLockState 两个槽函数内部逻辑完全一致,仅操作的状态变量不同,严重违反DRY(Don't Repeat Yourself)原则。此外,新增的 enableLockStateMonitoringnotifyLockStateChange 等关键底层函数均缺少注释说明其用途、参数含义及XKB协议相关背景。
潜在问题:后续维护时若修改去重或发射逻辑,容易遗漏其中一个函数导致状态不一致;缺少注释降低代码可读性
建议:提取一个通用的 updateLockState(uint &lastState, bool on, const QObject *context, const char *signal) 私有方法处理去重与发射逻辑;补充XKB事件解析相关的注释

  • 3.代码性能(无性能问题)✓

代码采用基于XCB事件驱动的监听模式,仅在系统产生XKB事件时触发回调,未引入轮询机制。notifyLockStateChange 内部仅包含一次小内存的拷贝与位运算,开销极小。updateNumLockState 等函数通过整型比较进行去重,时间复杂度为O(1),无性能瓶颈。
建议:保持现有的事件驱动架构即可

  • 4.代码安全(存在1个安全漏洞)✕

漏洞对比统计:新增漏洞 1 个,减少漏洞 0 个,持平 0 个
总体风险描述:在处理XCB底层事件时,存在堆缓冲区越界读取风险,攻击面依赖于恶意构造的X Server响应或内存损坏场景

  • 安全漏洞1(中危):[堆缓冲区越界读取] 在 [x11keyhandler.cpp 的 notifyLockStateChange 函数] 中,[使用 std::memcpy 按 xkbStateNotify 结构体大小(大于 xcb_generic_event_t 的32字节)读取 event 指针指向的内存。如果传入的 event 实际分配内存不足(如被恶意X Server截断),将导致越界读取堆内存,可能造成敏感信息泄露或程序崩溃(段错误)] ——非常重要

  • 建议:移除 std::memcpy,改用 reinterpret_cast<const xkbStateNotify*>(event) 直接转换指针并访问成员,这是处理XCB嵌套事件的标准惯用法,由调用方保证 xcb_poll_for_event 返回的内存有效且完整

■ 【改进建议代码示例】

diff --git a/src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.cpp b/src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.cpp
index ee0cae8..safe_version 100644
--- a/src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.cpp
+++ b/src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.cpp
@@ -968,17 +968,16 @@ void X11KeyHandler::handleKeyPress(const xcb_key_press_event_t *event)
 
+// 解析 XKB StateNotify 事件,提取并发射锁键状态变更信号
 void X11KeyHandler::notifyLockStateChange(const xcb_generic_event_t *event)
 {
     if (!event)
         return;
 
-    static_assert(sizeof(xkbStateNotify) == sz_xkbStateNotify);
-    xkbStateNotify stateEvent{};
-    std::memcpy(&stateEvent, event, sizeof(stateEvent));
-    if (stateEvent.xkbType != XkbStateNotify
-            || !(stateEvent.changed & XkbModifierLockMask)) {
+    // 使用 reinterpret_cast 替代 memcpy,避免越界读取风险,符合 XCB 事件处理规范
+    const auto *stateEvent = reinterpret_cast<const xkbStateNotify *>(event);
+    if (stateEvent->xkbType != XkbStateNotify
+            || !(stateEvent->changed & XkbModifierLockMask)) {
         return;
     }
 
-    emit numLockStateChanged(stateEvent.lockedMods & m_numLockMask);
-    emit capsLockStateChanged(stateEvent.lockedMods & m_capsLockMask);
+    emit numLockStateChanged(stateEvent->lockedMods & m_numLockMask);
+    emit capsLockStateChanged(stateEvent->lockedMods & m_capsLockMask);
 }
diff --git a/src/plugin-qt/shortcut/src/core/keybindingmanager.cpp b/src/plugin-qt/shortcut/src/core/keybindingmanager.cpp
index 050907b..deduplicate_version 100644
--- a/src/plugin-qt/shortcut/src/core/keybindingmanager.cpp
+++ b/src/plugin-qt/shortcut/src/core/keybindingmanager.cpp
@@ -1285,24 +1285,17 @@ void KeybindingManager::onCaptureKeyEvent(bool pressed, const QString &keystroke
     emit KeyEvent(pressed, keystroke);
 }
 
-void KeybindingManager::updateNumLockState(bool on)
+// 统一的锁键状态更新逻辑,包含去重判断与信号发射
+void KeybindingManager::updateLockState(uint &lastState, bool on, void (KeybindingManager::*signalPtr)(uint))
 {
-    const uint state = on ? 1U : 0U;
-    if (m_lastNumLockState == state)
+    const uint state = on ? 1U : 0U;
+    if (lastState == state)
         return;
 
-    m_lastNumLockState = state;
-    emit NumLockStateChanged(state);
-}
+    lastState = state;
+    emit (this->*signalPtr)(state);
+}
 
-void KeybindingManager::updateCapsLockState(bool on)
+void KeybindingManager::updateNumLockState(bool on)
 {
-    const uint state = on ? 1U : 0U;
-    if (m_lastCapsLockState == state)
-        return;
-
-    m_lastCapsLockState = state;
-    emit CapsLockStateChanged(state);
+    updateLockState(m_lastNumLockState, on, &KeybindingManager::NumLockStateChanged);
 }
+
+void KeybindingManager::updateCapsLockState(bool on)
+{
+    updateLockState(m_lastCapsLockState, on, &KeybindingManager::CapsLockStateChanged);
+}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants