1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
|
From 16f63a3c8fa227625bade5a9edea22354b347d18 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Barnab=C3=A1s=20P=C5=91cze?= <pobrn@protonmail.com>
Date: Fri, 18 Feb 2022 18:36:36 +0100
Subject: [PATCH] Revert "loop: remove destroy list"
This reverts commit c474846c42967c44db069a23b76a29da6f496f33.
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
Upstream-Status: Backport [https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/16f63a3c]
Signed-off-by: Scott Murray <scott.murray@konsulko.com>
---
spa/plugins/support/loop.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/spa/plugins/support/loop.c b/spa/plugins/support/loop.c
index 0588ce770..04739eb2a 100644
--- a/spa/plugins/support/loop.c
+++ b/spa/plugins/support/loop.c
@@ -75,6 +75,7 @@ struct impl {
struct spa_system *system;
struct spa_list source_list;
+ struct spa_list destroy_list;
struct spa_hook_list hooks_list;
int poll_fd;
@@ -325,6 +326,14 @@ static void loop_leave(void *object)
impl->thread = 0;
}
+static inline void process_destroy(struct impl *impl)
+{
+ struct source_impl *source, *tmp;
+ spa_list_for_each_safe(source, tmp, &impl->destroy_list, link)
+ free(source);
+ spa_list_init(&impl->destroy_list);
+}
+
static int loop_iterate(void *object, int timeout)
{
struct impl *impl = object;
@@ -354,11 +363,14 @@ static int loop_iterate(void *object, int timeout)
}
for (i = 0; i < nfds; i++) {
struct spa_source *s = ep[i].data;
- if (SPA_LIKELY(s && s->rmask)) {
+ if (SPA_LIKELY(s && s->rmask && s->loop)) {
s->priv = NULL;
s->func(s);
}
}
+ if (SPA_UNLIKELY(!spa_list_is_empty(&impl->destroy_list)))
+ process_destroy(impl);
+
return nfds;
}
@@ -712,7 +724,7 @@ static void loop_destroy_source(void *object, struct spa_source *source)
spa_system_close(impl->impl->system, source->fd);
source->fd = -1;
}
- free(source);
+ spa_list_insert(&impl->impl->destroy_list, &impl->link);
}
static const struct spa_loop_methods impl_loop = {
@@ -783,6 +795,8 @@ static int impl_clear(struct spa_handle *handle)
spa_list_consume(source, &impl->source_list, link)
loop_destroy_source(impl, &source->source);
+ process_destroy(impl);
+
spa_system_close(impl->system, impl->ack_fd);
spa_system_close(impl->system, impl->poll_fd);
@@ -844,6 +858,7 @@ impl_init(const struct spa_handle_factory *factory,
impl->poll_fd = res;
spa_list_init(&impl->source_list);
+ spa_list_init(&impl->destroy_list);
spa_hook_list_init(&impl->hooks_list);
impl->buffer_data = SPA_PTR_ALIGN(impl->buffer_mem, MAX_ALIGN, uint8_t);
--
2.35.1
|
