Fix native SIGSEGV when hold() races with release() on NativeMemoryManager - #12714
Fix native SIGSEGV when hold() races with release() on NativeMemoryManager#12714yikf wants to merge 1 commit into
Conversation
|
@zhztheplayer could you please take a look if you have time, thanks. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in NativeMemoryManager where hold() could call into native code after release() had freed the underlying native handle, leading to a JVM crash (SIGSEGV) under concurrent task teardown and iterator close.
Changes:
- Add an instance-level lock to make
hold()andrelease()mutually exclusive around native JNI calls. - Make
hold()a safe no-op when the memory manager has already been released, preventing use-after-free.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@yikf Thanks. I thought |
|
Thanks @zhztheplayer. You're right that in the normal flow the iterator is closed before the
In the normal single-threaded path both run on the task thread, and
So |
|
btw, i found this case at other PR ci pipeline, https://github.com/apache/gluten/actions/runs/31071855879/job/92523319304?pr=12697 |
|
@zhztheplayer friendly re-ping, please take a look again if you have time. |
|
@yikf Would you help check the failed CI? |
|
@zhztheplayer It also failed due to similar issues. I submitted a PR: #12740 |
b8927e8 to
2fcd5db
Compare
127a8b2 to
e407538
Compare
|
@yikf Thanks. Based on your context, this looks more like a Spark issue. As a broader bug might become the feed thread cannot access the SparkContext because it is stopped by the task thread, no matter Gluten is enabled or not. If that's the case, we shouldn't handle the race silently but rather throw an error. What do you think? |
|
@zhztheplayer You're right — this is essentially a Spark issue. In vanilla Spark that's relatively benign, but with Gluten, it escalates from a Java-level error into a native coredump — more severe than the plain-Spark case. Given that, I think either handling is reasonable — silently no-op'ing or throwing. I've switched to throwing so the broken teardown ordering is surfaced rather than hidden. Could you take another look? |
e407538 to
0fb5ceb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
gluten-arrow/src/main/scala/org/apache/gluten/memory/NativeMemoryManager.scala:90
- To fully prevent the SIGSEGV described in the PR, release() also needs to be mutually exclusive with hold() at the JNI boundary. Otherwise a thread can enter hold() (before released=true) and race with NativeMemoryManagerJniWrapper.release(handle). Wrapping the native release call with the same synchronized monitor used by hold() prevents concurrent native hold/release on the same handle.
NativeMemoryManagerJniWrapper.hold(handle)
}
override def getHandle(): Long = handle
override def release(): Unit = {
if (!released.compareAndSet(false, true)) {
| override def hold(): Unit = { | ||
| // hold() must run before release(). Reaching here after release means a broken teardown | ||
| // ordering, so surface it instead of dereferencing a freed native handle silently. | ||
| if (released.get()) { | ||
| throw new GlutenException( | ||
| s"Cannot hold memory manager instance that has already been released: $handle") | ||
| } | ||
| NativeMemoryManagerJniWrapper.hold(handle) | ||
| } |
0fb5ceb to
423c946
Compare
423c946 to
a6d7253
Compare
What changes are proposed in this pull request?
NativeMemoryManager.hold()called the nativehold(handle)JNI method without checking whether the manager had already been released. When a task tears down its runtime (release()frees the native handle) while another thread is still closing an output iterator viaColumnarBatchOutIterator.close0() -> memoryManager().hold(),hold()dereferences a freed handle and crashes the JVM with a SIGSEGV (SEGV_MAPERR= use-after-free).Surfaced as a flaky native crash in CI running
GlutenSparkScriptTransformationSuite(itsTRANSFORM ... USINGtests close the columnar output iterator on a feed thread concurrently with task teardown):From
hs_err_pid*.log:How was this patch tested?
flaky test, existed test to verify.
Was this patch authored or co-authored using generative AI tooling?
Yes, AI-assisted, Generated-by: Claude claude-opus-4-8.