pipewire: don't treat per-object errors as connection loss, don't tear down a shared context - #246
Merged
Merged
Conversation
pw_core_events::error carries the id of the proxy the error belongs to: PW_ID_CORE (0) is the connection itself, any other id is a per-object error. on_core_error() discarded `id` and judged every error as if it described the socket. The daemon emits per-object errors routinely. Two reproducible ones, both -ENOENT and both delivered on a non-core proxy id: pw_registry_bind() on a global that vanished between the registry event and the bind -> "no global <id>" pw_core_create_object() with a factory the daemon does not have, the shape a pw_stream takes when its autoconnect target is not up yet -> "unknown factory name <name>" Either one flipped the whole context to connection_state::broken, so ok() went false and synchronize() failed on a connection that was perfectly healthy. Callers react to `broken` by reconnecting, which tears down the loop every other holder of a shared context is using -- this misclassification is what set that crash in motion. Scope the handler to id == PW_ID_CORE, and drop -ENOENT from the fatal set even there: it means "no such object/factory", which is recoverable. Connection loss surfaces as -EPIPE / -ECONNRESET / -ENOTCONN. finalize_sync() no longer latches `broken` on its own either; whether an error is fatal is on_core_error()'s decision, and a round-trip that fails is not by itself a dead connection. tests/integration/pipewire_context_error_scope.cpp covers both triggers and asserts the context stays connected and usable.
shared_context() hands one process-wide connection to independent
subsystems -- the MIDI backends, an audio engine, video capture and
output devices. Each gets a shared_ptr and none of them can see the
others.
context::reconnect() called tear_down(), which does pw_core_disconnect
+ pw_context_destroy + pw_thread_loop_destroy and rebuilds, notifying
nobody. Every pw_stream and proxy another holder had created on that
core was left pointing at freed memory; the next pw_stream_destroy
faulted on the freed loop. One holder deciding to reconnect crashed
the others.
Two changes, because neither alone is enough:
- reconnect() refuses while more than one shared holder is alive.
This is the hard guarantee: a holder that ignores everything else
in this API still cannot free a loop another one is using.
Notification alone would not do -- it is cooperative, and an
unmodified holder would still crash.
- on_invalidated() fires from reconnect() immediately before
teardown, on the calling thread, while the loop, context and core
are still valid. That is the window in which a holder can destroy
what it built on them. The gate alone would not do either: it does
not protect a sole holder's own streams from its own reconnect.
Holders are counted explicitly rather than through use_count(), which
counts unrelated temporaries and cannot be checked atomically against
the teardown. shared_context() now returns a pointer carrying a holder
token; copies of one holder's pointer share the token and count once.
Contexts built with make() or borrow() are not shared and report zero
holders, so reconnect() behaves as before for them.
Recovery contract for a shared connection that genuinely died: holders
release it (or rebuild via on_invalidated) and reconnect succeeds once
one is left; rebuilding underneath live holders is not an option that
can be made safe.
tests/integration/pipewire_context_shared_teardown.cpp puts a real
pw_stream on the shared core from a second holder, has the first
holder attempt a reconnect, and asserts the loop and core it is using
survive -- then destroys the stream, which is the call that crashed in
the field.
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
1 participant
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.
Two related defects in the PipeWire backend's shared context. Together they form one
cascade: a routine per-object
-ENOENTis misread as connection loss, which triggers areconnect, which frees a
pw_thread_loopother holders are still using — SIGSEGV.1.
on_core_errorignored theidargumentpw_core_events::errordocuments (pipewire/core.h:128-146) that "the id argument isthe proxy object where the error occurred", and
PW_ID_CORE == 0. The handler discardedidand latchedconnection_state::brokenfor any of-EPIPE / -ECONNRESET / -ENOENT / -ENOTCONN, regardless of which object failed.The daemon emits per-object errors routinely. Observed on a live daemon:
id=37, not0— a healthy socket marked broken. Any client binding a stale global, orrequesting a factory that isn't present, poisoned the whole shared context.
Fixed by scoping the check to
id == PW_ID_CORE.-ENOENTis also dropped from the fatalset even on the core: it means "no such object/factory", not a dead socket — genuine loss
surfaces as
-EPIPE/-ECONNRESET/-ENOTCONN. Additionallyfinalize_sync()nolonger latches
brokenon its own, since a per-object error could poison the sync pathby the same route.
2.
reconnect()tore down a context other holders still useshared_context()is a process-wide singleton, butreconnect()didpw_core_disconnect+pw_context_destroy+pw_thread_loop_destroyand rebuilt,notifying nobody. Every
pw_streamanother holder built on that core was left pointing atfreed memory:
Fixed with two mechanisms, because neither is sufficient alone:
reconnect()refuses while more than one holder is registered — but that alone does notprotect a sole holder's own streams;
on_invalidated()callback fires before teardown, while handles are still valid —but notification alone is cooperative, so an unmodified holder would still crash.
Holders are counted with an explicit token handed out by
shared_context()rather thanuse_count(), which counts temporaries and cannot be checked atomically against theteardown.
Verification
pipewire_context_*ones. Both guards werenegative-controlled by disabling them individually: gate off → SegFault; callback off →
on_invalidated() did not fire before teardown.pipewire_context_{sync,reconnect,subscriptions}tests are unaffected — they usecontext::make(), which reports zero holders, soreconnect()behaves as before.virtual in/out open, send works.
Found while debugging a crash in ossia/score, where
three independent subsystems (two video devices and the audio engine) share one context.