DynamoRIO / DynamoRIO/dynamorio

Signals received by threads during the synch process can deadlock

Open
#3,262 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
3.2k
Forks
629
Avg merge
2d 18h
Merged PRs (30d)
30

Description

In PR #3249 we moved the removal of itimer handlers to earlier in the detach sequence because it was causing a deadlock we noticed internally. The race went something like:

0. Thread A has executed `dr_app_setup()` and `dr_app_start()`; Thread B is executing from the code-cache
1. Thread A initiates detach via `dr_app_stop_and_cleanup()` and attempts to synch with thread B; it increments thread B's `pending_synch_count`
2. On its next trip through dispatch, thread B blocks in `check_wait_at_safe_spot()` after setting its synch permission to `THREAD_SYNCH_VALID_MCONTEXT`
3. Thread B receives a signal (`SIGPROF`, or really any arbitrary signal sent to this thread which calls through `record_pending_signal`)
4. In `record_pending_signal`, thread B obtains the `fcache_unit_areas` readlock
5. Thread A then sends the `SUSPEND_SIGNAL` to thread B
6. Thread B receives the `SUSPEND_SIGNAL` (nested in the previous `SIGPROF`, while thread B still holds the `fcache_unit_areas` readlock)
7. Thread A synchs with thread B, finds that thread B has a safe synch perm from `check_wait_at_safe_spot`, and sends thread B native
8. At some point in the future, thread A attempts to obtain the `fcache_unit_areas` writelock and blocks forever

We somewhat mitigated this in PR #3249 in that the SIGPROF in question could not be received after a thread is blocked in `check_wait_at_safe_spot` for detach, but there are still races which are possible today:
* Thread B could have been recently blocking in `check_wait_at_safe_spot` from a previous synch when a signal comes and causes the previous set of events to occur.
* Many signals cause the `fcache_unit_areas` to be accessed, and any signal can be sent to any thread at any time by some other process not under DR control. Disabling just timer handlers only protects us from just a subset of signals, though in practice it's unlikely for this to affect a large number of use cases.

Contributor guide

Open the contributing guide

Research direction

Start at dr_app_stop_and_cleanup(), check_wait_at_safe_spot(), and record_pending_signal(), then compare the detach changes in PR #3249. Done means the described signal and detach interleavings no longer leave fcache_unit_areas blocked indefinitely; the issue does not name a file or test, so locate the relevant synchronization coverage first.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
operating-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.