Skip to content

Commit c9a9e2f

Browse files
fjtrujyclaude
andcommitted
fix: [ee/libcglue] correct __retarget_lock_acquire_recursive race
The previous implementation incremented lock->count and stored the caller's thread_id BEFORE WaitSema, with this race-prone shape: starting = lock->count == 0; if (starting) lock->thread_id = thread_id; // <-- racy if (lock->thread_id == thread_id) { lock->count++; if (starting) WaitSema(lock->sem_id); } else { WaitSema(lock->sem_id); lock->thread_id = thread_id; lock->count++; } If a higher-priority thread preempted between `count++` and `WaitSema`, the preempting thread saw `lock->count > 0`, took the else branch (another WaitSema), then nobody signalled it after the original thread released. Rewritten as a proper recursive mutex: - Fast path: if this thread already owns the gate (count > 0 && thread_id == self), just increment count and return. Safe to read those fields without the gate because only the owning thread mutates them. - Slow path: WaitSema first, *then* set thread_id + count = 1 atomically (relative to other threads, since we hold the gate). Same logic applied to __retarget_lock_try_acquire_recursive (PollSema instead of WaitSema, returns 1 on contention). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 586784c commit c9a9e2f

1 file changed

Lines changed: 27 additions & 36 deletions

File tree

ee/libcglue/src/lock.c

Lines changed: 27 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -140,26 +140,26 @@ void __retarget_lock_acquire(_LOCK_T lock)
140140
#ifdef F___retarget_lock_acquire_recursive
141141
void __retarget_lock_acquire_recursive(_LOCK_T lock)
142142
{
143-
bool starting = false;
144143
int32_t thread_id = GetThreadId();
145-
starting = lock->count == 0;
146-
if (starting) {
147-
lock->thread_id = thread_id;
148-
}
149-
if (lock->thread_id == thread_id) {
150-
lock->count++;
151-
if (starting) {
152-
WaitSema(lock->sem_id);
153-
}
154-
} else {
155-
WaitSema(lock->sem_id);
156-
// Reached here means that the lock was acquired by another thread
157-
// so now we need to make it ours
158-
// We can't put the lock->count++ before the WaitSema because it will
159-
// cause a deadlock
160-
lock->thread_id = thread_id;
144+
145+
/* Recursive case: this thread already owns the gate. We can read
146+
* thread_id/count without the gate because only the owning thread
147+
* mutates them while it holds the gate, and only the owning thread
148+
* can take this branch. */
149+
if (lock->count > 0 && lock->thread_id == thread_id) {
161150
lock->count++;
151+
return;
162152
}
153+
154+
/* Otherwise acquire the gate for real. Only after WaitSema returns
155+
* do we touch thread_id/count — putting count++ before WaitSema is
156+
* what causes the classic recursive-mutex deadlock here, because a
157+
* higher-priority thread preempting between count++ and WaitSema
158+
* sees count>0, falls into the else branch, races past the sema, and
159+
* then nobody signals it after release. */
160+
WaitSema(lock->sem_id);
161+
lock->thread_id = thread_id;
162+
lock->count = 1;
163163
}
164164
#endif
165165

@@ -173,28 +173,19 @@ int __retarget_lock_try_acquire(_LOCK_T lock)
173173
#ifdef F___retarget_lock_try_acquire_recursive
174174
int __retarget_lock_try_acquire_recursive(_LOCK_T lock)
175175
{
176-
int res = 0;
177-
bool starting = false;
178176
int32_t thread_id = GetThreadId();
179-
starting = lock->count == 0;
180-
if (starting) {
181-
lock->thread_id = thread_id;
182-
}
183-
if (lock->thread_id == thread_id) {
184-
lock->count++;
185-
if (starting) {
186-
res = PollSema(lock->sem_id) > 0 ? 0 : 1;
187-
}
188-
} else {
189-
res = PollSema(lock->sem_id) > 0 ? 0 : 1;
190-
// Reached here means that the lock was acquired by another thread
191-
// so now we need to make it ours
192-
// We can't put the lock->count++ before the WaitSema because it will
193-
// cause a deadlock
194-
lock->thread_id = thread_id;
177+
178+
if (lock->count > 0 && lock->thread_id == thread_id) {
195179
lock->count++;
180+
return 0;
181+
}
182+
183+
if (PollSema(lock->sem_id) <= 0) {
184+
return 1; /* gate not free */
196185
}
197-
return res;
186+
lock->thread_id = thread_id;
187+
lock->count = 1;
188+
return 0;
198189
}
199190
#endif
200191

0 commit comments

Comments
 (0)