From 2ca61880db1da2b15fe83d0fe94b95245ace77b0 Mon Sep 17 00:00:00 2001 From: James Westby Date: Wed, 11 Feb 2009 20:18:24 -0500 Subject: serialize removals, and avoid using freed data caused by removals I changed the code to also loop through watch->notifies when removing the watch due to inotify, and NULL each notify->watch reference, the code then checks this before trying to delete the watch itself if asked to remove the notify. In order to prevent other race conditions in this area I also made the inotify code not pass a watch to emit_events_in_idle, as the watch may get freed in the meantime. It instead passes the wd and the emit loop looks up the watch, discarding the event if the watch has been removed. I did however leave in the code that checks for a removed watch before doing anything with inotify, as I hoped that this would just optimise this case. --- src/ck-file-monitor-inotify.c | 126 +++++++++++++++++++++++++++++++----------- 1 file changed, 94 insertions(+), 32 deletions(-) (limited to 'src/ck-file-monitor-inotify.c') diff --git a/src/ck-file-monitor-inotify.c b/src/ck-file-monitor-inotify.c index 4fdd6aa..c4a11bd 100644 --- a/src/ck-file-monitor-inotify.c +++ b/src/ck-file-monitor-inotify.c @@ -55,7 +55,7 @@ typedef struct typedef struct { - FileInotifyWatch *watch; + int wd; CkFileMonitorEvent event; char *path; } FileMonitorEventInfo; @@ -80,7 +80,9 @@ struct CkFileMonitorPrivate guchar *buffer; guint events_idle_id; + guint remove_idle_id; GQueue *notify_events; + GQueue *remove_events; }; enum { @@ -242,6 +244,10 @@ static void file_monitor_remove_watch (CkFileMonitor *monitor, FileInotifyWatch *watch) { + if (watch->wd == -1) { + return; + } + g_hash_table_remove (monitor->priv->path_to_watch, watch->path); g_hash_table_remove (monitor->priv->wd_to_watch, @@ -296,6 +302,8 @@ static gboolean emit_events_in_idle (CkFileMonitor *monitor) { FileMonitorEventInfo *event_info; + char *path; + char *freeme; monitor->priv->events_idle_id = 0; @@ -303,29 +311,39 @@ emit_events_in_idle (CkFileMonitor *monitor) GSList *l; FileInotifyWatch *watch; - watch = event_info->watch; - - for (l = watch->notifies; l != NULL; l = l->next) { - FileMonitorNotify *notify; - - notify = g_hash_table_lookup (monitor->priv->notifies, - GUINT_TO_POINTER (l->data)); - if (notify == NULL) { - continue; - } - - if (! (notify->mask & event_info->event)) { - continue; - } + watch = g_hash_table_lookup (monitor->priv->wd_to_watch, + GINT_TO_POINTER (event_info->wd)); - if (notify->notify_func) { - notify->notify_func (monitor, event_info->event, event_info->path, notify->user_data); - } + if (watch != NULL) { + for (l = watch->notifies; l != NULL; l = l->next) { + FileMonitorNotify *notify; + + notify = g_hash_table_lookup (monitor->priv->notifies, + GUINT_TO_POINTER (l->data)); + if (notify == NULL) { + continue; + } + + if (! (notify->mask & event_info->event)) { + continue; + } + + if (notify->notify_func) { + freeme = NULL; + if (event_info->path != NULL) { + path = freeme = g_build_filename (watch->path, event_info->path, NULL); + } else { + path = watch->path; + } + notify->notify_func (monitor, event_info->event, path, notify->user_data); + if (freeme != NULL) { + g_free (freeme); + } + } + } } - g_free (event_info->path); event_info->path = NULL; - event_info->event = CK_FILE_MONITOR_EVENT_NONE; g_free (event_info); @@ -334,6 +352,36 @@ emit_events_in_idle (CkFileMonitor *monitor) return FALSE; } +static gboolean +emit_removals_in_idle (CkFileMonitor *monitor) +{ + gpointer wd_p; + FileInotifyWatch *watch; + GSList *l; + + monitor->priv->remove_idle_id = 0; + + while ((wd_p = g_queue_pop_head (monitor->priv->remove_events)) != NULL) { + watch = g_hash_table_lookup (monitor->priv->wd_to_watch, wd_p); + if (watch && watch->wd != -1) { + for (l = watch->notifies; l != NULL; l = l->next) { + FileMonitorNotify *notify; + + notify = g_hash_table_lookup (monitor->priv->notifies, + GUINT_TO_POINTER (l->data)); + if (notify == NULL) { + continue; + } + notify->watch = NULL; + } + file_monitor_remove_watch (monitor, watch); + g_free (watch); + } + } + + return FALSE; +} + static void file_monitor_queue_event (CkFileMonitor *monitor, FileMonitorEventInfo *event_info) @@ -347,7 +395,7 @@ file_monitor_queue_event (CkFileMonitor *monitor, static void queue_watch_event (CkFileMonitor *monitor, - FileInotifyWatch *watch, + int wd, CkFileMonitorEvent event, const char *path) { @@ -355,16 +403,26 @@ queue_watch_event (CkFileMonitor *monitor, event_info = g_new0 (FileMonitorEventInfo, 1); - event_info->watch = watch; + event_info->wd = wd; event_info->path = g_strdup (path); event_info->event = event; file_monitor_queue_event (monitor, event_info); } +static void +queue_remove_event (CkFileMonitor *monitor, + int wd) +{ + g_queue_push_tail (monitor->priv->remove_events, GINT_TO_POINTER (wd)); + + if (monitor->priv->remove_idle_id == 0) { + monitor->priv->remove_idle_id = g_idle_add ((GSourceFunc) emit_removals_in_idle, monitor); + } +} + static void handle_inotify_event (CkFileMonitor *monitor, - FileInotifyWatch *watch, struct inotify_event *ievent) { CkFileMonitorEvent event; @@ -375,9 +433,9 @@ handle_inotify_event (CkFileMonitor *monitor, freeme = NULL; if (ievent->len > 0) { - path = freeme = g_build_filename (watch->path, ievent->name, NULL); + path = ievent->name; } else { - path = watch->path; + path = NULL; } mask_str = imask_to_string (ievent->mask); @@ -397,11 +455,11 @@ handle_inotify_event (CkFileMonitor *monitor, } if (event != CK_FILE_MONITOR_EVENT_NONE) { - queue_watch_event (monitor, watch, event, path); + queue_watch_event (monitor, ievent->wd, event, path); } if (ievent->mask & IN_IGNORED) { - file_monitor_remove_watch (monitor, watch); + queue_remove_event (monitor, ievent->wd); } } @@ -460,7 +518,7 @@ inotify_data_pending (GIOChannel *source, watch = g_hash_table_lookup (monitor->priv->wd_to_watch, GINT_TO_POINTER (ievent->wd)); if (watch != NULL) { - handle_inotify_event (monitor, watch, ievent); + handle_inotify_event (monitor, ievent); } i += sizeof (struct inotify_event) + ievent->len; @@ -523,11 +581,13 @@ file_monitor_remove_notify (CkFileMonitor *monitor, g_hash_table_steal (monitor->priv->notifies, GUINT_TO_POINTER (id)); - notify->watch->notifies = g_slist_remove (notify->watch->notifies, GUINT_TO_POINTER (id)); + if (notify->watch) { + notify->watch->notifies = g_slist_remove (notify->watch->notifies, GUINT_TO_POINTER (id)); - if (g_slist_length (notify->watch->notifies) == 0) { - file_monitor_remove_watch (monitor, notify->watch); - g_free (notify->watch); + if (g_slist_length (notify->watch->notifies) == 0) { + file_monitor_remove_watch (monitor, notify->watch); + g_free (notify->watch); + } } g_free (notify); @@ -629,6 +689,7 @@ ck_file_monitor_init (CkFileMonitor *monitor) monitor->priv->serial = 1; monitor->priv->notify_events = g_queue_new (); + monitor->priv->remove_events = g_queue_new (); setup_inotify (monitor); } @@ -649,6 +710,7 @@ ck_file_monitor_finalize (GObject *object) g_hash_table_destroy (monitor->priv->notifies); g_queue_free (monitor->priv->notify_events); + g_queue_free (monitor->priv->remove_events); G_OBJECT_CLASS (ck_file_monitor_parent_class)->finalize (object); } -- cgit