Skip to content

Commit 7ac1583

Browse files
committed
frontend/darwin: fix type-pun UB and over-release of global GCD queue in path watcher
frontend_darwin_watch_path_for_changes had two distinct bugs in its cleanup paths. [...]
1 parent ed41ed5 commit 7ac1583

1 file changed

Lines changed: 65 additions & 29 deletions

File tree

frontend/drivers/platform_darwin.m

Lines changed: 65 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,50 @@ static bool accessibility_speak_macos(int speed,
882882
#endif
883883

884884
#ifdef HAVE_GCD
885+
/* Tear down the per-watcher darwin_watch_data_t: cancel every
886+
* dispatch source, close every file descriptor, free the copied
887+
* path strings, free the watches array, and free watch_data itself.
888+
*
889+
* Does NOT release watch_data->queue: that slot holds the handle
890+
* returned by dispatch_get_global_queue(), which is a process-wide
891+
* singleton that must never be released - dispatch_release on a
892+
* global queue is documented as undefined behaviour (on pre-10.8
893+
* SDKs where OS_OBJECT_USE_OBJC=0 it over-decrements the refcount;
894+
* on newer SDKs it's a no-op macro under ARC, so the bug has been
895+
* latent but real). The earlier teardown code called
896+
* 'dispatch_release(watch_data->queue)' under !__has_feature(objc_arc);
897+
* that call has been removed here.
898+
*
899+
* Does NOT touch the path_change_data_t wrapper that points at
900+
* watch_data; that's the caller's responsibility. */
901+
static void darwin_watch_data_free(darwin_watch_data_t *watch_data)
902+
{
903+
size_t i;
904+
905+
if (!watch_data)
906+
return;
907+
908+
if (watch_data->watches)
909+
{
910+
for (i = 0; i < watch_data->watch_count; i++)
911+
{
912+
if (watch_data->watches[i].source)
913+
{
914+
dispatch_source_cancel(watch_data->watches[i].source);
915+
#if !__has_feature(objc_arc)
916+
dispatch_release(watch_data->watches[i].source);
917+
#endif
918+
}
919+
if (watch_data->watches[i].fd >= 0)
920+
close(watch_data->watches[i].fd);
921+
if (watch_data->watches[i].path)
922+
free(watch_data->watches[i].path);
923+
}
924+
free(watch_data->watches);
925+
}
926+
free(watch_data);
927+
}
928+
885929
static void frontend_darwin_watch_path_for_changes(
886930
struct string_list *list, int flags,
887931
path_change_data_t **change_data)
@@ -894,33 +938,8 @@ static void frontend_darwin_watch_path_for_changes(
894938
if (!change_data || !*change_data)
895939
return;
896940

897-
watch_data = (darwin_watch_data_t*)((*change_data)->data);
898-
if (watch_data)
899-
{
900-
size_t i;
901-
/* Cancel and release all dispatch sources, close file descriptors */
902-
for (i = 0; i < watch_data->watch_count; i++)
903-
{
904-
if (watch_data->watches[i].source)
905-
{
906-
dispatch_source_cancel(watch_data->watches[i].source);
907-
#if !__has_feature(objc_arc)
908-
dispatch_release(watch_data->watches[i].source);
909-
#endif
910-
}
911-
if (watch_data->watches[i].fd >= 0)
912-
close(watch_data->watches[i].fd);
913-
if (watch_data->watches[i].path)
914-
free(watch_data->watches[i].path);
915-
}
916-
#if !__has_feature(objc_arc)
917-
if (watch_data->queue)
918-
dispatch_release(watch_data->queue);
919-
#endif
920-
if (watch_data->watches)
921-
free(watch_data->watches);
922-
free(watch_data);
923-
}
941+
darwin_watch_data_free(
942+
(darwin_watch_data_t*)((*change_data)->data));
924943
free(*change_data);
925944
*change_data = NULL;
926945
return;
@@ -1013,8 +1032,25 @@ static void frontend_darwin_watch_path_for_changes(
10131032
(*change_data)->data = watch_data;
10141033
else
10151034
{
1016-
/* Failed to allocate change_data, cleanup */
1017-
frontend_darwin_watch_path_for_changes(NULL, 0, &(path_change_data_t*){watch_data});
1035+
/* path_change_data_t wrapper alloc failed. The previous code
1036+
* called frontend_darwin_watch_path_for_changes(NULL, 0,
1037+
* &(path_change_data_t*){watch_data})
1038+
* i.e. it passed a fake path_change_data_t pointer that
1039+
* actually pointed at watch_data itself (a darwin_watch_data_t).
1040+
* The teardown branch then read '(*change_data)->data' from
1041+
* that pointer, which happened to be the 'queue' field of
1042+
* darwin_watch_data_t (both are void*-sized at offset 0).
1043+
* So watch_data got set to the global dispatch queue pointer;
1044+
* the subsequent for-loop walked off the end of that system-
1045+
* owned struct interpreting arbitrary bytes as watch_count /
1046+
* watches[], and the final free() free()d the shared global
1047+
* queue. The only reason it has not caused user-visible
1048+
* breakage is that this OOM path is ~never taken in practice.
1049+
*
1050+
* Replaced with a direct call to darwin_watch_data_free which
1051+
* operates on a real darwin_watch_data_t* without the fake
1052+
* wrapper. */
1053+
darwin_watch_data_free(watch_data);
10181054
}
10191055
}
10201056

0 commit comments

Comments
 (0)