fix(dxinput): use global mutex to prevent X11 thread race condition #173
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, list.c, type.c, property.c, button_map.c, and keyboard.c each had their own local mutex, which failed to protect concurrent X11 access across different files. This caused heap corruption when multiple threads simultaneously called XOpenDisplay/XCloseDisplay, resulting in "malloc(): unaligned tcache chunk detected" errors. Log: - Add x11_mutex.h/c with global x11_global_mutex - Replace all local mutexes with x11_global_mutex - Add _unlocked internal versions of query_device_type and is_property_exist to avoid deadlock when called from within locked contexts - Ensure all top-level functions (called from Go) acquire the lock - Internal helper functions assume caller holds the lock Influence: This ensures thread-safe X11 access while avoiding deadlock through proper lock hierarchy. --- 修复(dxinput): 使用全局互斥锁防止 X11 线程竞争 之前 list.c、type.c、property.c、button_map.c 和 keyboard.c 各自 使用独立的局部互斥锁，无法保护跨文件的并发 X11 访问。这导致多个线程 同时调用 XOpenDisplay/XCloseDisplay 时发生堆损坏，触发 "malloc(): unaligned tcache chunk detected" 错误。 Log： - 新增 x11_mutex.h/c，定义全局 x11_global_mutex - 将所有局部互斥锁替换为 x11_global_mutex - 新增 query_device_type 和 is_property_exist 的 _unlocked 内部版本， 避免在已持锁的上下文中调用时发生死锁 - 确保所有顶层函数（被 Go 调用）获取锁 - 内部辅助函数假设调用者已持有锁 Influence: 通过正确的锁层次结构，确保线程安全的 X11 访问，同时避免死锁。
Reviewer's GuideIntroduces a single global X11 mutex shared across all dxinput C modules, replaces local per-file mutexes, and refactors selected APIs into locked/unlocked variants to enforce a consistent lock hierarchy and prevent cross-file X11 race conditions, plus minor safety tweaks in the Go wrapper. Sequence diagram for Go property query with global X11 mutexsequenceDiagram
actor GoGoroutine
participant GoUtils as Go_utils.wrapper
participant CUtils as C_dxinput_utils
participant Mutex as x11_global_mutex
participant X11 as X11_server
GoGoroutine->>GoUtils: IsPropertyExist(id, prop)
GoUtils->>CUtils: is_property_exist(id, prop)
activate CUtils
CUtils->>Mutex: pthread_mutex_lock(x11_global_mutex)
activate Mutex
Mutex-->>CUtils: lock_acquired
deactivate Mutex
CUtils->>CUtils: is_property_exist_unlocked(id, prop)
CUtils->>X11: XOpenDisplay()
activate X11
X11-->>CUtils: Display*
CUtils->>X11: XIListProperties(deviceid)
X11-->>CUtils: props[], nprops
loop iterate_properties
CUtils->>X11: XGetAtomName(disp, atom)
X11-->>CUtils: name
CUtils->>CUtils: strcmp(name, prop)
CUtils->>X11: XFree(name)
end
CUtils->>X11: XCloseDisplay(disp)
deactivate X11
CUtils->>Mutex: pthread_mutex_unlock(x11_global_mutex)
Mutex-->>CUtils: lock_released
CUtils-->>GoUtils: bool_exist
deactivate CUtils
GoUtils-->>GoGoroutine: bool_exist
Class diagram for dxinput X11 mutex and utility modulesclassDiagram
class X11MutexModule {
+pthread_mutex_t x11_global_mutex
}
class TypeModule {
+int query_device_type(int deviceid)
+int query_device_type_unlocked(int deviceid)
+int is_property_exist(int deviceid, const char* prop)
-int is_property_exist_unlocked(int deviceid, const char* prop)
-int is_mouse_device(int deviceid)
-int is_touchpad_device(int deviceid)
-int is_touchscreen_device(int deviceid)
-int is_wacom_device(int deviceid)
-int is_keyboard_device(int deviceid)
}
class PropertyModule {
+unsigned char* get_prop(int id, const char* prop, int* nitems)
+int set_prop_int(int id, const char* prop, unsigned char* data, int nitems, int bit)
+int set_prop_float(int id, const char* prop, unsigned char* data, int nitems)
+int set_prop(int id, const char* prop, unsigned char* data, int nitems, Atom type, int format)
}
class ButtonMapModule {
+unsigned char* get_button_map(unsigned long xid, const char* name, int* nbuttons)
+int set_button_map(unsigned long xid, const char* name, unsigned char* map, int nbuttons)
-int get_button_number(Display* disp, const char* name)
-int get_device_button_number(const XDeviceInfo* dev)
-unsigned char* do_get_button_map(Display* disp, unsigned long xid, int nbuttons)
-const XDeviceInfo* find_device_by_name(const XDeviceInfo* devs, int ndevs, const char* name)
}
class ListModule {
+DeviceInfo* list_device(int* num)
-int append_device(DeviceInfo** devs, XIDeviceInfo* xinfo, int idx)
-void free_device_info(DeviceInfo* dev)
}
class KeyboardModule {
+int set_keyboard_repeat(int repeated, unsigned int delay, unsigned int interval)
}
class GoWrapper {
+bool IsPropertyExist(int32 id, string prop)
+[]byte GetProperty(int32 id, string prop)
+error SetInt8Prop(int32 id, string prop, []int8 values)
+error SetInt16Prop(int32 id, string prop, []int16 values)
+error SetInt32Prop(int32 id, string prop, []int32 values)
+error SetFloat32Prop(int32 id, string prop, []float32 values)
+[]byte ucharArrayToByte(*C.uchar cData, int length)
}
TypeModule ..> X11MutexModule : uses
PropertyModule ..> X11MutexModule : uses
ButtonMapModule ..> X11MutexModule : uses
ListModule ..> X11MutexModule : uses
KeyboardModule ..> X11MutexModule : uses
ListModule ..> TypeModule : calls_query_device_type_unlocked
GoWrapper ..> TypeModule : calls_query_device_type
GoWrapper ..> PropertyModule : calls_get_set_prop
GoWrapper ..> ButtonMapModule : calls_button_map
GoWrapper ..> ListModule : calls_list_device
GoWrapper ..> KeyboardModule : calls_set_keyboard_repeat
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
GetProperty(wrapper.go), the new early return whencdatas == nil || nitems == 0skips freeingcdatas, which can leak the X11-allocated buffer whencdatas != nilbutnitems == 0; consider freeingcdatasbefore returning in that case to match the other paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GetProperty` (wrapper.go), the new early return when `cdatas == nil || nitems == 0` skips freeing `cdatas`, which can leak the X11-allocated buffer when `cdatas != nil` but `nitems == 0`; consider freeing `cdatas` before returning in that case to match the other paths.
## Individual Comments
### Comment 1
<location> `dxinput/utils/wrapper.go:209` </location>
<code_context>
func ucharArrayToByte(cData *C.uchar, length int) []byte {
</code_context>
<issue_to_address>
**suggestion:** The nil check on cdata inside ucharArrayToByte is redundant
Since cdata is derived from cData via pointer arithmetic, it cannot be nil for 0 <= i < length if cData itself is non-nil. The `if cdata == nil { break }` branch is therefore dead code. Consider removing it, or instead validating that cData (the argument) is non-nil upfront if that’s the intent.
</issue_to_address>
### Comment 2
<location> `dxinput/utils/wrapper.go:114` </location>
<code_context>
}()
nitems := C.int(0)
cdatas := C.get_prop(C.int(id), cprop, &nitems)
- if cdatas == nil {
+ if cdatas == nil || nitems == 0 {
</code_context>
<issue_to_address>
**question (bug_risk):** Treating nitems == 0 as an error changes the semantics of GetProperty for empty properties
This change makes `GetProperty` return `(nil, 0)` when the property exists but is empty (non-null data pointer, `nitems == 0`), removing the distinction between “absent” (nil) and “empty” (zero-length slice). If callers rely on that distinction, consider only treating `cdatas == nil` as an error and returning an explicit empty slice (e.g. `make([]byte, 0)`) when `nitems == 0` but `cdatas != nil`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| cdata := (*C.uchar)(unsafe.Pointer(uintptr(unsafe.Pointer(cData)) + uintptr(i)*cItemSize)) | ||
| offset := uintptr(i) * cItemSize | ||
| addr := uintptr(unsafe.Pointer(cData)) + offset | ||
| cdata := (*C.uchar)(unsafe.Pointer(addr)) |
There was a problem hiding this comment.
suggestion: The nil check on cdata inside ucharArrayToByte is redundant
Since cdata is derived from cData via pointer arithmetic, it cannot be nil for 0 <= i < length if cData itself is non-nil. The if cdata == nil { break } branch is therefore dead code. Consider removing it, or instead validating that cData (the argument) is non-nil upfront if that’s the intent.
|
|
||
| nitems := C.int(0) | ||
| cdatas := C.get_prop(C.int(id), cprop, &nitems) | ||
| if cdatas == nil { |
There was a problem hiding this comment.
question (bug_risk): Treating nitems == 0 as an error changes the semantics of GetProperty for empty properties
This change makes GetProperty return (nil, 0) when the property exists but is empty (non-null data pointer, nitems == 0), removing the distinction between “absent” (nil) and “empty” (zero-length slice). If callers rely on that distinction, consider only treating cdatas == nil as an error and returning an explicit empty slice (e.g. make([]byte, 0)) when nitems == 0 but cdatas != nil.
mhduiy
approved these changes
Jan 6, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, mhduiy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, list.c, type.c, property.c, button_map.c, and keyboard.c each had their own local mutex, which failed to protect concurrent X11 access across different files. This caused heap corruption when multiple threads simultaneously called XOpenDisplay/XCloseDisplay, resulting in "malloc(): unaligned tcache chunk detected" errors.
Log:
Influence: This ensures thread-safe X11 access while avoiding deadlock through proper lock hierarchy.
修复(dxinput): 使用全局互斥锁防止 X11 线程竞争
之前 list.c、type.c、property.c、button_map.c 和 keyboard.c 各自 使用独立的局部互斥锁，无法保护跨文件的并发 X11 访问。这导致多个线程
同时调用 XOpenDisplay/XCloseDisplay 时发生堆损坏，触发 "malloc(): unaligned tcache chunk detected" 错误。
Log：
Influence: 通过正确的锁层次结构，确保线程安全的 X11 访问，同时避免死锁。
Summary by Sourcery
Introduce a global X11 mutex to serialize all X11 operations across dxinput utilities and prevent cross-file race conditions.
Bug Fixes:
Enhancements: