fix(core): recover from SIGKILL race in unlock_shrreg() - #260
fix(core): recover from SIGKILL race in unlock_shrreg()#260Princess0407 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Princess0407 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 |
📝 WalkthroughWalkthroughThe shared-region semaphore lock now recovers when a process clears ChangesSemaphore recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 790-800: The ownerless semaphore recovery branch in the timeout
loop must be made cross-process exclusive and bounded. Around the current_owner
== 0 recovery in the relevant wait function, add a shared recovery claim so only
one waiter can perform the forced sem_post, check and handle sem_post failure,
and track/limit recovery attempts so they cannot continue indefinitely before
the existing trials > 30 failure path; preserve the existing retry behavior when
recovery is successfully claimed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d39a26-b0b8-444d-bf70-da2a6dac5b78
📒 Files selected for processing (1)
src/multiprocess/multiprocess_memory_limit.c
| // RECOVERY: unlock_shrreg() race where owner_pid was cleared | ||
| // but sem_post() never executed | ||
| // if owner_pid == 0 and we are still timing out, the semaphore | ||
| // is stuck locked with no owner. Force a post to unstick it. | ||
| if (current_owner == 0 && trials > 5) { | ||
| LOG_WARN("Detected stuck semaphore with owner_pid == 0 " | ||
| "(possible unlock race), forcing recovery post"); | ||
| sem_post(®ion->sem); | ||
| usleep(10000); // 10ms for semaphore to propagate | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'sem_init|pshared|SEM_WAIT_TIME|SEM_WAIT_RETRY_TIMES|sem_timedwait|sem_post|owner_pid|lock_shrreg|unlock_shrreg' \
src/multiprocessRepository: Project-HAMi/HAMi-core
Length of output: 43127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
src = Path("src/multiprocess/multiprocess_memory_limit.c").read_text()
m = re.search(r"void\s+lock_shrreg\(\)\s*\{(?P<body>.*?)\n\}", src, re.S)
if not m:
raise SystemExit("lock_shrreg not found")
body = m.group("body")
print("lock_shrreg sem_timedwait_count:", body.count("sem_timedwait(®ion->sem"))
print("lock_shrreg owner_pid_set_count:", body.count("region->owner_pid = region_info.pid"))
print("lock_shrreg recovery_post_count:", len(re.findall(r"sem_post\(®ion->sem\);", body)))
print("lock_shrreg continuation_count:", body.count("continue;"))
checks = {
"sem_timedwait_before_owner_publish": "sem_timedwait(®ion->sem" in body and "region->owner_pid = region_info.pid" in body,
"recovery_executes_after_timedwait": "etimedout" in body.lower() or "ETIMEDOUT" in body,
"recovery_continues": "sem_post(®ion->sem)" in body and body.find("sem_post(®ion->sem)") > body.find("current_owner == 0 && trials > 5"),
}
for k, v in checks.items():
print(f"{k}: {v}")
init = Path("src/multiprocess/multiprocess_shrreg.c").read_text() if Path("src/multiprocess/multiprocess_shrreg.c").exists() else ""
if init:
print("init sem_init_count:", len(re.findall(r"sem_init\(®ion->sem,\s*1,\s*1\)", init)))
print("owner_pid_zero_init_nearby:", re.search(r"owner_pid\s*=\s*0", init))
else:
init = src
print("no init file; sem_init occurrences in memory_limit.c:", len(re.findall(r"sem_init\(®ion->sem,\s*1,\s*1\)", src)))
PY
sed -n '1090,1170p' src/multiprocess/multiprocess_memory_limit.cRepository: Project-HAMi/HAMi-core
Length of output: 3926
Make ownerless semaphore recovery owned and bounded.
current_owner == 0 only shows that no owner was published after sem_timedwait() timed out. Every timeout after trial 5 can take this branch, and multiple waiters can each execute sem_post(®ion->sem). Add a cross-process recovery claim around the post, check the return value of sem_post(), and limit the recovery attempts before the existing trials > 30 failure path.
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 795-795: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/multiprocess/multiprocess_memory_limit.c` around lines 790 - 800, The
ownerless semaphore recovery branch in the timeout loop must be made
cross-process exclusive and bounded. Around the current_owner == 0 recovery in
the relevant wait function, add a shared recovery claim so only one waiter can
perform the forced sem_post, check and handle sem_post failure, and track/limit
recovery attempts so they cannot continue indefinitely before the existing
trials > 30 failure path; preserve the existing retry behavior when recovery is
successfully claimed.
When a process is SIGKILL'd between clearing owner_pid and posting the semaphore in unlock_shrreg(), the semaphore remains locked with no owner. lock_shrreg()'s recovery logic only handled dead owners (current_owner != 0), leaving the lock stuck forever when current_owner == 0. This change adds a recovery branch in lock_shrreg() that detects owner_pid == 0 after repeated timeouts and forces a sem_post() to unstick pending waiters. Also replaces the vague TODO with a descriptive comment explaining the race and recovery path. Related: #2500, #2125, solves #2500 Signed-off-by: Priyanka Tiwari <priyankatiwari140419@gmail.com>
44f5912 to
1b826d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 792-794: Replace the sem_getvalue()/sem_val < 0 heuristic in the
current_owner recovery path with a shared atomic recovery claim/state. Ensure
only one process can claim recovery after repeated trials, including when
owner_pid is cleared but sem_post was interrupted, then perform the existing
semaphore recovery without relying on waiter counts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75e9d51e-f030-446d-9274-047b074d10a8
📒 Files selected for processing (1)
src/multiprocess/multiprocess_memory_limit.c
| if (current_owner == 0 && trials > 5) { | ||
| int sem_val; | ||
| if (sem_getvalue(®ion->sem, &sem_val) == 0 && sem_val < 0) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i 'linux|ubuntu|debian|alpine|glibc|musl' \
--glob 'Dockerfile*' --glob '*.yml' --glob '*.yaml' --glob '*.md' . || true
if command -v man >/dev/null 2>&1; then
MANPAGER=cat man 3 sem_getvalue | grep -E 'zero|negative|Linux adopts|changed'
else
echo "Verify sem_getvalue behavior in the target build image."
fiRepository: Project-HAMi/HAMi-core
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## Candidate files\n'
git ls-files | rg 'Dockerfile|\.ya?ml|README\.md|src/multiprocess/multiprocess_memory_limit\.c' || true
printf '\n## Target docs references\n'
rg -n -i 'multiprocess|HAMi|gpu|linux|ubuntu|debian|alpine|glibc|musl|docker|container' \
--glob 'Dockerfile*' --glob '*.yml' --glob '*.yaml' --glob '*.md' . || true
printf '\n## Relevant C source excerpts\n'
wc -l src/multiprocess/multiprocess_memory_limit.c
sed -n '720,850p' src/multiprocess/multiprocess_memory_limit.c
printf '\n## sem_getvalue usage and related recovery code\n'
rg -n 'sem_getvalue|sem_post|recover|trials|owner|SIGKILL|multiprocess_memory_limit' src/multiprocess/multiprocess_memory_limit.cRepository: Project-HAMi/HAMi-core
Length of output: 16655
🌐 Web query:
man sem_getvalue POSIX Linux sem_getvalue returns non-negative value zero number of processes blocked
💡 Result:
Under the POSIX standard, if one or more processes or threads are blocked on a semaphore, the value placed in the sval argument by sem_getvalue may either be 0 or a negative number whose absolute value represents the count of blocked processes or threads [1][2][3]. Linux explicitly adopts the former behavior: when processes or threads are blocked, sem_getvalue returns 0 in the location pointed to by sval [1][4][5]. It does not provide the count of blocked processes as a negative number [1][6]. The function sem_getvalue itself returns 0 on success and -1 on error (with errno set) [1][2][4]. Note that any value retrieved by sem_getvalue may already be outdated by the time the application receives it, as the state of the semaphore can change immediately after the call [1][5][3].
Citations:
- 1: https://man7.org/linux/man-pages/man3/sem_getvalue.3.html
- 2: https://man7.org/linux/man-pages/man3/sem_getvalue.3p.html
- 3: https://pubs.opengroup.org/onlinepubs/9799919799/functions/sem_getvalue.html
- 4: https://man.archlinux.org/man/sem_getvalue.3.en
- 5: https://manned.org/man/sem_getvalue.3
- 6: https://manned.org/man/ubuntu-jammy/sem_getvalue.3
Use a shared recovery claim instead of sem_val < 0.
sem_getvalue() does not reliably indicate blocked waiters. POSIX allows 0 for blocked waiters, and Linux reports 0 rather than a negative count. If SIGKILL interrupts unlock_shrreg() after clearing owner_pid but before sem_post(), this condition leaves the semaphore locked, so waiters keep timing out instead of breaking out. Track recovery with shared atomic state/state rather than inferring the stuck lock from sem_getvalue().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/multiprocess/multiprocess_memory_limit.c` around lines 792 - 794, Replace
the sem_getvalue()/sem_val < 0 heuristic in the current_owner recovery path with
a shared atomic recovery claim/state. Ensure only one process can claim recovery
after repeated trials, including when owner_pid is cleared but sem_post was
interrupted, then perform the existing semaphore recovery without relying on
waiter counts.
|
This is being closed because it does not comply with the contribution guidelines. |
2 participants
Description
unlock_shrreg()has a microsecond race window betweenowner_pid = 0andsem_post(). If the kernel delivers SIGKILL in that window, the semaphore stays locked forever butowner_pidis already cleared.lock_shrreg()recovery only handledcurrent_owner != 0(dead owner cleanup), so all waiting processes hung indefinitely.This PR adds recovery for the
current_owner == 0case after repeated timeouts, forcing asem_post()to unstick the semaphore. It also replaces the vague// TODO: irregular exit here will hang pending lockswith a proper comment.Changes
lock_shrreg(): addcurrent_owner == 0 && trials > 5recovery branchunlock_shrreg(): replace vague TODO with descriptive commentWhy not swap sem_post() and owner_pid = 0?
If
sem_post()runs first, a new acquirer can setowner_pidbefore the original unlocker clears it overwriting the legitimate owner. Recovery in the waiter is the safe design.Related
Summary by CodeRabbit