[PR #36] [MERGED] fix(cuda): take own ref on GstCudaContext to fix double-unref at teardown #37

New issue

Closed

opened 2026-10-15 12:47:47 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 12:47:47 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/games-on-whales/gst-wayland-display/pull/36
Author: @salty2011
Created: 6/19/2026
Status: ✅ Merged
Merged: 8/19/2026
Merged by: @ABeltramo

Base: stable ← Head: fix/dmabuftocuda-context-teardown

📝 Commits (1)

ce354c1 fix(cuda): adopt the reference the gst_cuda helpers' short-circuit never transferred

📊 Changes

1 file changed (+70 additions, -0 deletions)

View changed files

📝 wayland-display-core/src/utils/allocator/cuda/mod.rs (+70 -0)

📄 Description

Problem

gst_cuda_ensure_element_context() and gst_cuda_handle_set_context() both open with:

/* If we had context already, will not replace it */
if (*cuda_ctx)
  return TRUE;

They report success while transferring nothing. Every other success path is (transfer full) into the slot. CUDAContext::drop unconditionally unrefs self.ptr, so a wrapper built off that short-circuit destroys a reference it never owned.

The compositor hits this on its own context:

new_from_gstreamer() calls the helper, which writes the slot then publishes the context.
That publish re-enters waylanddisplaysrc::set_context() synchronously, from inside the same call.
At re-entry the settings slot is still None, so the inner, non-owning wrapper is stored and the outer one, holding the single legitimate reference, is dropped when the outer call returns.
The session then runs with the context held only by the published GstContext. At teardown that drops, the refcount hits zero, and the GstCudaContext is finalized while the pipeline's stored GstContext still points at it.
Disposing the pipeline walks element->contexts and lands on freed memory: SIGSEGV in g_type_check_instance_is_fundamentally_a, at the end of every session teardown.

Only bites when nothing else outlives the pipeline. A host that pins one GstCudaContext for the process lifetime keeps the refcount above zero and the defect stays invisible.

Fix

adopt_slot_ref() takes a reference only on the short-circuit, at both constructors.

Detection is unambiguous because both helpers write the slot only when it was NULL on entry: success with a non-NULL pre-value is necessarily the short-circuit, success with a NULL pre-value is necessarily a transfer. The (transfer full) paths are untouched.

What changed since this PR was opened

Retargeted from feat/nv12-output to stable. That branch is divergent and unmerged; the defect is still present on stable at both constructors.
The fix approach changed. The original was an unconditional gst_object_ref() before gst_cuda_stream_new(). That fixes the SIGSEGV but adds a reference nothing removes on the normal transfer path, turning the crash into a per-session GstCudaContext leak. The conditional version is correct in both paths and is what has been running downstream.

Not addressed here

The slot itself is never cleared when its context dies, so a later call can read a stale pre-value. That hazard predates this change and is unaffected by it. Happy to take it separately if you want it closed.

Verified

cargo fmt clean, cargo check --features cuda clean, 23 tests pass. CUDAContext::new()'s own gst_cuda_stream_new is unaffected (no slot, no short-circuit possible) and is untouched.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/games-on-whales/gst-wayland-display/pull/36 **Author:** [@salty2011](https://github.com/salty2011) **Created:** 6/19/2026 **Status:** ✅ Merged **Merged:** 8/19/2026 **Merged by:** [@ABeltramo](https://github.com/ABeltramo) **Base:** `stable` ← **Head:** `fix/dmabuftocuda-context-teardown` --- ### 📝 Commits (1) - [`ce354c1`](https://github.com/games-on-whales/gst-wayland-display/commit/ce354c1985d66eecaa548db5048437a984f2afba) fix(cuda): adopt the reference the gst_cuda helpers' short-circuit never transferred ### 📊 Changes **1 file changed** (+70 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `wayland-display-core/src/utils/allocator/cuda/mod.rs` (+70 -0) </details> ### 📄 Description ## Problem `gst_cuda_ensure_element_context()` and `gst_cuda_handle_set_context()` both open with: ```c /* If we had context already, will not replace it */ if (*cuda_ctx) return TRUE; ``` They report success while transferring nothing. Every other success path is `(transfer full)` into the slot. `CUDAContext::drop` unconditionally unrefs `self.ptr`, so a wrapper built off that short-circuit destroys a reference it never owned. The compositor hits this on its own context: - `new_from_gstreamer()` calls the helper, which writes the slot then publishes the context. - That publish re-enters `waylanddisplaysrc::set_context()` **synchronously**, from inside the same call. - At re-entry the settings slot is still `None`, so the **inner, non-owning** wrapper is stored and the **outer** one, holding the single legitimate reference, is dropped when the outer call returns. - The session then runs with the context held only by the published `GstContext`. At teardown that drops, the refcount hits zero, and the `GstCudaContext` is finalized while the pipeline's stored `GstContext` still points at it. - Disposing the pipeline walks `element->contexts` and lands on freed memory: SIGSEGV in `g_type_check_instance_is_fundamentally_a`, at the end of every session teardown. Only bites when nothing else outlives the pipeline. A host that pins one `GstCudaContext` for the process lifetime keeps the refcount above zero and the defect stays invisible. ## Fix `adopt_slot_ref()` takes a reference **only** on the short-circuit, at both constructors. Detection is unambiguous because both helpers write the slot only when it was NULL on entry: success with a non-NULL pre-value is necessarily the short-circuit, success with a NULL pre-value is necessarily a transfer. The `(transfer full)` paths are untouched. ## What changed since this PR was opened - **Retargeted from `feat/nv12-output` to `stable`.** That branch is divergent and unmerged; the defect is still present on `stable` at both constructors. - **The fix approach changed.** The original was an unconditional `gst_object_ref()` before `gst_cuda_stream_new()`. That fixes the SIGSEGV but adds a reference nothing removes on the normal transfer path, turning the crash into a per-session `GstCudaContext` leak. The conditional version is correct in both paths and is what has been running downstream. ## Not addressed here The slot itself is never cleared when its context dies, so a later call can read a stale pre-value. That hazard predates this change and is unaffected by it. Happy to take it separately if you want it closed. ## Verified `cargo fmt` clean, `cargo check --features cuda` clean, 23 tests pass. `CUDAContext::new()`'s own `gst_cuda_stream_new` is unaffected (no slot, no short-circuit possible) and is untouched. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-15 12:47:47 +00:00