The core of the issue is the following: what happens if an
active source is destroyed before it could be dispatched?
For loop-managed sources (`struct source_impl`) this was addressed
by storing all destroyed sources in a list, and only freeing them
after dispatching has been finished. (0eb73f0f06)
This approach works for both strictly single-threaded
and `pw_thread_loop` loops assuming the loop is not
reentered.
However, if the loop is reentered, there can still be issues.
Assume that in one iteration sources A and B are active,
and returned from the system call, and source B is destroyed
before the loop starts dispatching. Consider what happens when
"A" is dispatched first, and it reenters the loop with timeout 0.
Imagine there are no new events, so `loop_iterate()` will immediately
return, but it will first destroy everything in the destroy list
(this is done at the end of `loop_iterate()`).
And herein lies the problem. In the previous iteration,
there exists a `spa_poll_event` object which points to source "B",
but that has just been destroyed at the end of the recursive
iteration. This will trigger a use-after-free once the previous
iteration inspects it.
Fix that by processing the destroy list right after first
processing the returned `spa_poll_event` objects, and
"detach" the source from the loop and its iterations
in `process_destroy()` before the source is destroyed.
See #2114#2147
It may be a little confusing that both the loop object
and the `source_impl` objects are referred to with variables
named `impl`. For this reason, rename all source_impl objects
named `impl` to `s`.
It is expected that `nfds` is non-negative in the vast majority
of cases, so hopefully the runtime performance will not be
significantly affected by removing the check. This way
it is guaranteed that the destroy list is processed.
This reverts commit c474846c42.
In addition, `s->loop` is also checked before dispatching a source.
The destroy list is needed in the presence of threads. The
issue is that a source may be destroyed between `epoll_wait()`
returning and thread loop lock being acquired. If this
source is active, then a use-after-free will be triggered
when the thread loop acquires the lock and starts dispatching
the sources.
thread 1 thread 2
---------- ----------
loop_iterate
spa_loop_control_hook_before
// release lock
pw_thread_loop_lock
spa_system_pollfd_wait
// assume it returns with source A
pw_loop_destroy_source(..., A)
// frees storage of A
pw_thread_loop_unlock
spa_loop_control_hook_after
// acquire the lock
for (...) {
struct spa_source *s = ep[i].data;
s->rmask = ep[i].events;
// use-after-free if `s` refers to
// the previously freed `A`
Fixes#2147
Add an extra private field to the source to store the pollevent of
the current iteration. This changes ABI but it seems an embedded source
is not used outside of our own plugins and the unit test doesn't test
this ABI case.
Whenever a source is removed, we can set the data field of the
pollevent to NULL so that it won't be handled in any iteration anymore.
Avoid dispatching the same event multiple times when doing recursive
iterations.
Add some more unit tests for this.
Fixes#2114
This commit adds a counter for loop_enter/leave and checks:
- consecutive enter are used on the same thread
- leave is used on the same thread as enter
- at destruction, the enter_count must be 0
Now that sources can't be dispatched anymore after a _remove, we don't
need to keep the destroy_list anymore and we can free the source
immediately.
See #2114
Keep the array of dispatched sources around in the loop. When a source
is removed while dispatching, set the data to NULL so that we don't try
to deref the source again or call its function.
Fixes#2114
The ringbuffer can't be written to from multiple threads.
When both the main loop and data thread do _invoke, they both write to
the ringbuffer and cause it to be corrupted because the ringbuffer is
not multi-writer safe.
Doing invoke from the thread itself is usually done to flush things out
so we really only need to flush the ringbuffer and call the callback.
See #1451
First calculate the size of the aligned payload and then check if
we can fit this aligned payload in the remaining space in the
ringbuffer.
Otherwise we might be able to fit the item + payload in the remaining
space but then place the alignment bytes at the begginning, which would
break alignment of the next invoke_item struct.
Also check if there is enough space to write the payload bytes.
We check if there is enough space for the invoke_item structure first.
Then we calculate how much bytes we need to use for the payload but we
fail to check if we can actually write that much data, risking
overwriting existing data from the ringbuffer and causing a crash later
when we try to jump to invalid memory.
Add some more comments.
SPA_MEMBER is misleading, all we're doing here is pointer+offset and a
type-casting the result. Rename to SPA_PTROFF which is more expressive (and
has the same number of characters so we don't need to re-indent).
If we are already in the loop thread and flushing, this means we
added a new invoke item on the list from a callback. Place the
item on the queue and let the flush code take care of it after the
callback completes.
Required to fix some issues with draining in pulse where a stream
is destroyed from the drained callback which then invokes a pause.
Because the signal can't be removed from the callback we can
simply iterate backwards and then forwards.
The first added hook (the unlock/lock pair) is called last before
going into the poll and first when leaving. This executes all other
callbacks inside a locked situation. And removing them with the lock
is not going to cause problems.
Use a safer version of the before and after hooks. First call
all before hooks and save them in reverse order in a save list.
Then call the after event for the ones remaining in the save list
and move them back to the hook list.
This makes it possible to remove the hooks from one the callbacks or
even from other threads with the right locks. Found as a solution to
the following problem as observed in vlc:
main thread thread_loop
pw_thread_loop_lock() before hook: lock suspend thread
pw_context_destroy()
- removes before hook to flush clients
pw_thread_loop_unlock()
before hook: lock acquired, resume
before hook: flush client hook executed
*crash*
pw_thread_loop_stop()
pw_thread_loop_destroy()
Any of the safer cursor methods (like spa_hook_list_call()) would also
work but are more expensive and don't reverse the before/after
order.
This is more in line with wayland and it allows us to create new
interfaces in modules without having to add anything to the type
enum. It also removes some lookups to map type_id to readable
name in debug.
When we do invoke from the loop thread, first process the pending
invoke items before handling ours. This can be used to sync the
invoke queue before cleanup.
Define a set of standard factory names and document what they
contain. This makes it possible to change the implementation by
mapping the factory-name to a different shared library.
Move the epoll functions to the system functions and make the loop
use those. Use simple mask for events instead of enum.
Add the used system api in pw_loop.
Add System API to spa_support and use it where possible.
Pass the system API used in the realtime loops in spa_support as
well and use this in the realtime paths.
Improve bootstrapping, load only the log and cpu interfaces because
those can/need to be shared between instances. Let the core load
the other interfaces.
Add keys to configure the System and Loop implementations used in
pw_loop.
Make a new API to hide some the implementation of eventfd, timerfd
and signalfd along with clock and read/write/ioctl/close functions.
We would like to have plugins use the abstractions so that we
can switch them to something else when needed.
The interface struct has the type,version and methods of the
interface.
Make spa interfaces extend from spa_interface and make a
separate structure for the methods.
Pass a generic void* as the first argument of methods, like
we don in PipeWire.
Bundle the methods + implementation in a versioned inteface
and use that to invoke methods. This way we can do version
checks on the methods.
Make resource and proxy interfaces that we can can call. We
can then make the core interfaces independent on proxy/resource and
hide them in the lower layers.
Add add_listener method to methods of core interfaces, just
like SPA.
