Commit a04f966
committed
frontend/drivers/platform_darwin: fix dispatch_source_cancel UAF in path watcher teardown
The GCD-based directory watcher in platform_darwin.m had a
latent use-after-free in its teardown path.
=== The bug ===
darwin_watch_data_free was calling dispatch_source_cancel
followed immediately by free(watch_data):
dispatch_source_cancel(watch_data->watches[i].source);
...
free(watch_data);
This is unsafe because dispatch_source_cancel is asynchronous.
Per Apple's dispatch documentation, cancel() only marks the
source as cancelled - any event handler invocation that was
already dispatched to the source's target queue will run to
completion. The cancel handler is guaranteed to fire only
AFTER all pending event handler invocations have drained.
The event handler in question captures watch_data by pointer
and writes to it:
dispatch_source_set_event_handler(source, ^{
OSAtomicCompareAndSwap32(0, 1, &watch_data->has_changes);
});
Because the target queue is the global concurrent dispatch
queue (dispatch_get_global_queue at line 953), event handlers
run on any of the system worker threads. If the main thread
is inside darwin_watch_data_free while a kqueue event fires
for the watched vnode, the sequence is:
thread A (main) thread B (global queue)
------------------- -------------------------
dispatch_source_cancel
dispatch_release (event handler dispatched
free(watch_data) before cancel took effect)
OSAtomicCompareAndSwap32(
0, 1, &watch_data->
has_changes);
/* WRITES TO FREED MEMORY */
The old cancel handler was a no-op with a misleading comment:
dispatch_source_set_cancel_handler(source, ^{
/* File descriptor is closed in cleanup function */
});
This gave the impression that cleanup was synchronised, but
in fact nothing blocked the main thread until the cancel
handler had actually fired.
=== Reachability ===
The watcher is installed by video_shader_parse_preset to
monitor .slangp/.cgp files and their included passes for
changes. frontend_driver_watch_path_for_changes(NULL, 0, ...)
is called on every preset reload, shader restart, or user
navigation away from the current preset. Under normal
interactive use (user saves a shader file, then immediately
navigates the menu) the race window is readily reached - the
kqueue event for the save is in flight while the menu action
tears down the watcher.
On modern macOS this often crashes immediately because the
freed memory is immediately re-used by the allocator. On
iOS/tvOS the same race exists but crashes less often due to
different allocator behaviour.
=== The fix ===
Add a per-source dispatch_semaphore_t cancel_sem to
darwin_watch_entry_t. The cancel handler signals it; the
teardown path cancels then waits on the semaphore before
freeing. This is the canonical safe-teardown pattern for
dispatch sources:
1. dispatch_source_set_cancel_handler signals cancel_sem.
2. darwin_watch_data_free:
- dispatch_source_cancel(source)
- dispatch_semaphore_wait(cancel_sem, FOREVER)
- RARCH_DISPATCH_RELEASE on source and semaphore
- close fd, free path, free watch entries
- free watch_data
By the time dispatch_semaphore_wait returns, GCD guarantees
no event handler for this source is executing or will execute
again. The subsequent free(watch_data) is safe.
=== Semaphore lifetime ===
The cancel handler block captures cancel_sem. On both ARC
and MRC-with-OS_OBJECT_USE_OBJC (the default on modern
macOS/iOS) the block retains the captured dispatch object,
so the semaphore survives until the block does. Even without
block-level retention, we also hold cancel_sem through
watch_data->watches[i].cancel_sem until AFTER the wait has
returned, so the pointer passed to dispatch_semaphore_signal
is always valid.
=== Semaphore creation failure ===
dispatch_semaphore_create can in theory fail on OOM
(realistically rare). If it does, we skip source creation
entirely rather than create a source we cannot safely
cancel. Also added cleanup release on the
source-creation-failure path so the semaphore doesn't leak
when dispatch_source_create fails but the semaphore
succeeded.
=== ARC/MRC compatibility ===
Uses RARCH_DISPATCH_RELEASE from libretro-common/include/
defines/cocoa_defines.h instead of an inline
'#if !__has_feature(objc_arc)' + dispatch_release guard.
The macro is a no-op under ARC and 'do { if (x) dispatch_
release(x); } while (0)' under MRC. Three advantages over
the inline guard:
1. Single-line call instead of a 3-line ifdef block.
2. NULL-guarded automatically - dispatch_release(NULL) is
explicitly undefined per Apple's docs; the macro makes
it impossible to forget the check.
3. Safe on pre-10.8 SDKs where OS_OBJECT_USE_OBJC=0 and
dispatch types are plain C handles (the cocoa_defines.h
comment explains why '[x release]' is the wrong choice
for dispatch objects).
This matches the only other user in the tree
(record/drivers/record_avfoundation.m line 160), keeping the
macro's usage consistent across .m files that release
dispatch objects.
Added '#include <defines/cocoa_defines.h>' inside the
existing '#ifdef HAVE_GCD' block at the top of the file,
matching the placement convention for GCD-specific headers.
=== HAVE_GCD guards ===
All edits are inside existing '#ifdef HAVE_GCD' blocks
(struct definition at line 134, darwin_watch_data_free at
line 897, and frontend_darwin_watch_path_for_changes at
line 962). No HAVE_GCD scope changes - builds without GCD
(non-macOS/iOS darwin targets) are unaffected.
=== Thread-safety ===
darwin_watch_data_free is called from the main thread during
shader preset teardown. dispatch_semaphore_wait with
DISPATCH_TIME_FOREVER is blocking but bounded: the cancel
handler runs on the global concurrent queue which always has
worker threads available, and the cancel handler itself does
almost nothing (one semaphore signal), so the wait is
measured in microseconds in practice.
If somehow the cancel handler never fires (hypothetical
GCD bug), the main thread would block forever - but the
same condition would also mean leaked memory and dangling
watches in the existing code, so the new behaviour is
strictly better.
=== Not changing ===
The event handler body is unchanged. OSAtomicCompareAndSwap32
on &watch_data->has_changes is still correct - we've just
made sure the pointer is valid for every invocation. The
field is int32_t volatile and the atomic primitive provides
the necessary memory ordering with frontend_darwin_check_for_
path_changes which reads the flag on the main thread.
=== Verification ===
Brace balance verified on both modified functions. The
'continue' statement added to skip a watch when semaphore
creation fails lands inside the 'for (i = 0; i < list->size;
i++)' loop, which is the intended scope.
Can't compile-test locally (no macOS SDK), but the diff is
localised and the changes match the canonical GCD
safe-teardown pattern documented by Apple.1 parent 608a182 commit a04f966
1 file changed
Lines changed: 70 additions & 12 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| 29 | + | |
29 | 30 | | |
30 | 31 | | |
31 | 32 | | |
| |||
137 | 138 | | |
138 | 139 | | |
139 | 140 | | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
140 | 154 | | |
141 | 155 | | |
142 | 156 | | |
| |||
911 | 925 | | |
912 | 926 | | |
913 | 927 | | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
| 937 | + | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
914 | 942 | | |
915 | | - | |
916 | | - | |
917 | | - | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
| 948 | + | |
| 949 | + | |
| 950 | + | |
918 | 951 | | |
919 | 952 | | |
920 | 953 | | |
| |||
984 | 1017 | | |
985 | 1018 | | |
986 | 1019 | | |
987 | | - | |
988 | | - | |
989 | | - | |
| 1020 | + | |
| 1021 | + | |
| 1022 | + | |
| 1023 | + | |
990 | 1024 | | |
991 | 1025 | | |
992 | 1026 | | |
993 | | - | |
| 1027 | + | |
| 1028 | + | |
994 | 1029 | | |
995 | 1030 | | |
996 | 1031 | | |
| 1032 | + | |
| 1033 | + | |
| 1034 | + | |
| 1035 | + | |
| 1036 | + | |
| 1037 | + | |
| 1038 | + | |
| 1039 | + | |
| 1040 | + | |
| 1041 | + | |
| 1042 | + | |
| 1043 | + | |
| 1044 | + | |
| 1045 | + | |
997 | 1046 | | |
998 | 1047 | | |
999 | 1048 | | |
| |||
1003 | 1052 | | |
1004 | 1053 | | |
1005 | 1054 | | |
1006 | | - | |
| 1055 | + | |
| 1056 | + | |
| 1057 | + | |
| 1058 | + | |
1007 | 1059 | | |
1008 | 1060 | | |
1009 | 1061 | | |
1010 | 1062 | | |
1011 | | - | |
| 1063 | + | |
| 1064 | + | |
| 1065 | + | |
| 1066 | + | |
1012 | 1067 | | |
1013 | | - | |
| 1068 | + | |
1014 | 1069 | | |
1015 | 1070 | | |
1016 | | - | |
| 1071 | + | |
| 1072 | + | |
1017 | 1073 | | |
1018 | 1074 | | |
1019 | 1075 | | |
1020 | 1076 | | |
1021 | | - | |
| 1077 | + | |
| 1078 | + | |
| 1079 | + | |
1022 | 1080 | | |
1023 | 1081 | | |
1024 | 1082 | | |
| |||
0 commit comments