PipeWire backend: connect the filter only once its port exists (supersedes #249) - #250
Conversation
The filter can start before the local port exists (open sequence) or run after the port was removed (close sequence). With no valid port token there is no buffer to process; skip instead of dereferencing a null token. With asserts compiled out (release build) the old code segfaulted (pw.filter_dequeue_buffer on a null token).
Follow-up to the process() guard: fix the ordering that made it necessary instead of only surviving it. The backends connected the filter in the constructor and added the local port in open_port(). The node carries node.always-process, so the daemon schedules it as soon as it is exported, ports or not (context.c makes an always-process node runnable on its own), and impl_node_process calls the user callback unconditionally. A midi_in/midi_out that is constructed and not opened immediately therefore runs process() against a null port token within milliseconds, and pw_filter_dequeue_buffer(nullptr) dereferences an invalid pointer. Reproduced on pipewire 1.6.8; the code path is the same in 0.3.48 and 1.0.5. close_port() had the mirror problem: pw_filter_remove_port frees the port on the caller's thread with no data-loop synchronization while the filter stays connected, so a process() in flight can race the free. The guard only narrows that window. - create_filter() no longer connects. A new start_filter() step runs after create_local_port() (and set_port_buffer() for outputs) and connects the filter, then waits for the node id as before. - close_port() and the destructor go through release_filter(): filter::stop() disconnects, which unprepares the node on the data loop synchronously via pw_impl_node_destroy, and pw_filter_destroy frees the port itself. The next open_port() creates a fresh filter. - Drop the asserts that sat above the new guard in the midi_in variants; they aborted debug builds before the guard could run. Verified with a header-only test on pipewire 1.6.8: unopened objects idle for seconds, 200 open/close cycles and 100 construct/open/destroy cycles pass with asserts on and off; the previous code crashed in the idle case every time and in the open/close case about one run in four. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuaGAzPuXJNAUNkNFPDXro
None of the existing pipewire tests touch the MIDI backends; they exercise the shared context and a raw filter. Add a standalone integration test in the same style (skips with exit 0 without a daemon, watchdog against hangs) that keeps an unopened midi_in/midi_out alive for 500 ms, runs 100 open/close cycles on both, and 30 construct/open/destroy cycles, then checks the shared context is still connected. Against master before the process() guard it segfaults in the idle step; against the guard alone with asserts enabled it aborts on the assert that sat above the guard. Passes on this branch with and without NDEBUG. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuaGAzPuXJNAUNkNFPDXro
|
Added
Full suite on this branch: 17/17 pass (the five |
|
Also verified on a second machine: Ubuntu 24.04.4 LTS, libpipewire 1.0.7 (the reporter's distro, one point release ahead of 1.0.5), gcc 13.3, running against the user's live daemon.
|
|
@ryanatseolaudio if you have time to check this PR? it should solve the issues across all cases. |
|
@jcelerier Bug appears to be squashed, thanks! |
|
great thanks ! |
Builds on #249 by @ryanatseolaudio (his commit is kept as the first commit of this branch) and fixes the ordering that made the guard necessary.
Problem
The guard in #249 is correct as far as it goes, but two things remained:
assert(this->flt); assert(this->port.valid());above the new guard, so with asserts enabled the abort atmidi_in.hpp:95was unchanged.open_port(). The node carriesnode.always-process, so the daemon schedules it as soon as it is exported, ports or not (context.cmakes an always-process node runnable on its own), andimpl_node_processcalls the user callback unconditionally. Amidi_in/midi_outconstructed and not opened immediately runsprocess()against a null token within milliseconds, andpw_filter_dequeue_buffer(nullptr)dereferences an invalid pointer (SPA_CONTAINER_OFon null). Same code path in PipeWire 0.3.48, 1.0.5 and 1.6.8.pw_filter_remove_portfrees the port on the caller's thread with no data-loop synchronization while the filter stays connected, so aprocess()in flight can checkport.valid()and then dequeue from freed memory. The guard only narrows that window.Fix
create_filter()no longer connects. A newstart_filter()step runs aftercreate_local_port()(and afterset_port_buffer()for outputs), connects the filter and waits for the node id as before. Ports and their params therefore exist beforepw_filter_connect, the same order PipeWire's own examples use.close_port()and the destructor go throughrelease_filter():filter::stop()disconnects, which unprepares the node on the data loop synchronously (pw_impl_node_destroy→do_node_unprepare), thenpw_filter_destroyfrees the port itself. The nextopen_port()creates a fresh filter.pw_filter_remove_portis no longer called on a live filter.Verification
Header-only test program against PipeWire 1.6.8, run with asserts on and with
NDEBUG:midi_inconstructed, never opened, 3 smidi_outconstructed, never opened, 3 sopen_virtual_port/close_portcycles (in)The pipewire examples and
tests/integration/pipewire_context_subscriptions.cppstill compile.🤖 Generated with Claude Code
https://claude.ai/code/session_01EuaGAzPuXJNAUNkNFPDXro