Skip to content

Read _reader under the lock in WindowsApiEmitter.queue_events() (#1170)#1175

Open
uttam12331 wants to merge 1 commit into
gorakhargosh:masterfrom
uttam12331:fix/winapi-emitter-reader-lock-1170
Open

Read _reader under the lock in WindowsApiEmitter.queue_events() (#1170)#1175
uttam12331 wants to merge 1 commit into
gorakhargosh:masterfrom
uttam12331:fix/winapi-emitter-reader-lock-1170

Conversation

@uttam12331

Copy link
Copy Markdown

Summary

Closes #1170.

WindowsApiEmitter.on_thread_stop() reads and clears self._reader while holding self._lock:

def on_thread_stop(self) -> None:
    with self._lock:
        reader = self._reader
        self._reader = None
    if reader is not None:
        reader.stop()

but queue_events() read self._reader without holding the same lock, so the two accesses to the shared attribute were unsynchronized.

Fix

Copy self._reader into a local while holding self._lock, mirroring on_thread_stop(), then use the local reference outside the lock (the reader.get_events() loop stays outside the lock, as before). This is the approach suggested in the issue.

Tests

Added test_queue_events_reads_reader_under_lock, which spies on the emitter's lock and asserts that queue_events() acquires it while reading _reader. It fails on master (the lock is never taken in queue_events()) and passes with this change. ruff check/ruff format are clean; added a changelog entry.

(The test is in the Windows-only test_observers_winapi.py module and was run on Windows.)

`on_thread_stop()` reads and clears `self._reader` while holding `self._lock`,
but `queue_events()` read `self._reader` without holding the lock, so the two
accesses to the shared attribute were unsynchronized.

Copy `self._reader` into a local while holding `self._lock` (mirroring
`on_thread_stop()`), then use the local reference outside the lock.

Closes gorakhargosh#1170
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.

Potential data race on WindowsApiEmitter._reader between on_thread_stop() and queue_events()

1 participant