Skip to content

Library: fix race conditions in device management - #687

Open
reinauer wants to merge 1 commit into
USBGuard:mainfrom
reinauer:race_conditions
Open

Library: fix race conditions in device management#687
reinauer wants to merge 1 commit into
USBGuard:mainfrom
reinauer:race_conditions

Conversation

@reinauer

Copy link
Copy Markdown

The _sysfs_path_to_id_map and _backlog structures were being accessed
concurrently by the main thread and the uevent thread without proper
synchronization. This could lead to memory corruption or erroneous
device rejections during the initial system scanning phase.

Fix this by:

  1. Protecting _sysfs_path_to_id_map in DeviceManagerBase using the
    existing refDeviceMapMutex().
  2. Protecting the event _backlog in UEventDeviceManager using a
    dedicated static mutex to ensure thread-safe buffering and handoff.

The _sysfs_path_to_id_map and _backlog structures were being accessed
concurrently by the main thread and the uevent thread without proper
synchronization. This could lead to memory corruption or erroneous
device rejections during the initial system scanning phase.

Fix this by:
1. Protecting _sysfs_path_to_id_map in DeviceManagerBase using the
   existing refDeviceMapMutex().
2. Protecting the event _backlog in UEventDeviceManager using a
   dedicated static mutex to ensure thread-safe buffering and handoff.

@Cropi Cropi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good direction, a few issues to address:

1. Deadlock in DeviceManagerBase
getIDFromSysfsPath and isPresentSysfsPath lock refDeviceMapMutex() then call knownSysfsPath(), which locks the same mutex again. std::mutex isn't recursive so this deadlocks. knownSysfsPath should be an internal unlocked helper.

2. Exception safety in backlog locking
Raw pthread_mutex_lock/unlock around emplace_back isn't safe — if it throws, the mutex stays locked. Use a member std::mutex with std::lock_guard instead.

3. Static mutex over instance data (minor)
G_backlog_mutex is static but _backlog is per-instance. No real issue since there's only ever one instance, but a member mutex would be cleaner.

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.

2 participants