Fix a crash caused by renaming a live worker thread from another thread - #4722
Open
saran2020 wants to merge 1 commit into
Open
Fix a crash caused by renaming a live worker thread from another thread#4722saran2020 wants to merge 1 commit into
saran2020 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CoroutineScheduler.Worker.indexInArray's setter unconditionally renames the underlyingThreadon every assignment:Two of its three call sites are safe: renaming a
Workerbeforestart()(no live peer yet), and a worker renaming itself during self-termination. The third, intryTerminateWorker(), renames a different, live worker as part of keeping the workers array dense after a pool shrink:On Android, renaming a live thread you don't own requires the ART runtime to suspend it first (
Thread.setName→Thread_setNativeName→SuspendThreadByPeer). If that thread doesn't reach a safepoint quickly enough — under GC pressure, device load, or anything else that delays it — the suspend attempt times out and ART aborts the whole process withSIGABRT. This matches the crash reports in #2234 exactly, including theCoroutineScheduler$Worker.tryTerminateWorker/setIndexInArrayframes in the native backtrace.I confirmed this is exploitable on-demand: forcing worker-pool churn while deliberately starving one worker (CPU priority + core-affinity pinning + blocking I/O + GC pressure) reproduces the exact crash reliably on an emulator, with the abort backtrace showing this precise call chain.
Fix
Only rename the thread synchronously when it's safe to do so — the thread hasn't started yet, or the calling thread is the thread being renamed:
When neither holds (the cross-thread case), the rename is deferred to the worker's own next loop iteration in
runWorker(), which is always a same-thread — and therefore always safe — rename:The numeric
indexInArrayfield still updates immediately and unconditionally in all cases, so scheduling/parking/work-stealing correctness is unaffected — only the display name can lag briefly after a pool shrink, until the affected worker next runs.Also updated
SchedulerTestBase.maxSequenceNumber()to readindexInArraydirectly instead of parsing it out ofThread.name, since the name can now be briefly stale right after a shrink event.Drawbacks/trade-offs
Worker names can be briefly stale after a pool shrink. When a cross-thread rename is deferred, the affected worker keeps showing its old index in
Thread.getName()— visible in thread dumps,jstack, Android Studio's profiler, ANR reports, or any logging that tags by thread name — until that worker next loops around inrunWorker(). In practice, this window is bounded by whatever the worker is currently doing (its current task, or until it wakes from parking), so it's on the order of the same task-scheduling latency that's already normal for this pool, not an unbounded delay. This is a real, if minor, change in observability: previously, the name was always immediately accurate; now it's eventually accurate.The crash itself isn't reproducible in a JVM unit test. The bug only manifests through ART's native thread-suspension machinery on Android, which doesn't exist in a plain JVM test environment — confirmed directly while working on this fix, since neither a JNI critical section nor CPU/IO starvation could force the equivalent failure outside of a real Android runtime. The added tests verify the actual safety property this fix relies on (a live thread is never renamed except by itself), not the crash/abort itself. If a future change reintroduces a similar cross-thread rename somewhere else in the scheduler, these tests won't catch it unless it goes through this exact code path.
Testing
WorkerRenameTestcovering both live-thread branches: a cross-threadindexInArraychange never renames synchronously (and the worker self-heals its name on its next run), and a worker renaming itself is applied synchronously.schedulingtest suite passes with no regressions.SIGABRT.Fixes:
#2234