GNOME Bugzilla – Bug 776147
gio/glocalfilemonitor.c doesn't handle case G_FILE_MONITOR_EVENT_MOVED, crashing many programs.
Last modified: 2019-07-15 02:51:12 UTC
Line 430 of gio/glocalfilemonitor.c doesn't handle case G_FILE_MONITOR_EVENT_MOVED, crashing many programs. The list of programs affected includes but is not limited to: xfdesktop https://bugs.launchpad.net/ubuntu/+source/xfdesktop4/+bug/1598360 pcmanfm https://bugs.launchpad.net/ubuntu/+source/pcmanfm/+bug/1598488 and caja https://bugs.launchpad.net/ubuntu/+source/caja/+bug/1598359 The affected code can be found at https://github.com/GNOME/glib/blob/master/gio/glocalfilemonitor.c and looks like this: switch (event_type) { (other conditions are located here) case G_FILE_MONITOR_EVENT_MOVED: /* was never available in this API */ default: g_assert_not_reached (); } This matches with the g_assertion_message() crash that these programs are experiencing. So, I think we've found the culprit, and the solution would be to do something to handle case G_FILE_MONITOR_EVENT_MOVED so that these programs do not crash. Alternatively, if this API function should never be used, then the maintainers of these programs should be notified of this easily reproducible bug and be shown how to fix it. Right now, many packages that come default on major distros such as Ubuntu flavors are completely broken.
*** Bug 790148 has been marked as a duplicate of this bug. ***
Why not to move: case G_FILE_MONITOR_EVENT_MOVED: under: case G_FILE_MONITOR_EVENT_RENAMED: so they will handle same way (because they are *almost* same right?). G_FILE_MONITOR_EVENT_RENAMED branch even operates with names like "G_FILE_MONITOR_WATCH_MOVES" and "G_FILE_MONITOR_SEND_MOVED" internally. Apparently "was never available in this API" is not true because of the list of affected apps.
Created attachment 363349 [details] [review] kqueue: Fix invalid emission of G_FILE_MONITOR_EVENT_MOVED event That event is deprecated, and the kqueue backend can’t provide enough information to go alongside the event (i.e. the name of the new file). Use G_FILE_MONITOR_EVENT_DELETED instead. Quite disappointed in the kqueue documentation for this: I cannot find a single piece of documentation or example about how NOTE_RENAME is supposed to communicate the new name of the file. If it turns out that this is possible, the code can be amended again in future. At least now it doesn’t abort. Signed-off-by: Philip Withnall <withnall@endlessm.com>
(In reply to Robert from comment #2) > Why not to move: > case G_FILE_MONITOR_EVENT_MOVED: > > under: > case G_FILE_MONITOR_EVENT_RENAMED: > > so they will handle same way (because they are *almost* same right?). > G_FILE_MONITOR_EVENT_RENAMED branch even operates with names like > "G_FILE_MONITOR_WATCH_MOVES" and "G_FILE_MONITOR_SEND_MOVED" internally. Because that would be fixing the symptom of the problem, rather than the underlying cause. This problem hasn’t been fixed already because nobody’s found time to investigate it yet. > Apparently "was never available in this API" is not true because of the list > of affected apps. I just spent a bit of time looking into it, and it seems this is caused by the kqueue backend sending through an invalid event. I’ve attached a patch for that. If you weren’t using FreeBSD (or another system which uses kqueue, like OS X), please let me know. There’s a potential bug in the inotify backend where a -1 return value from ih_mask_to_EventFlags() can be passed through to g_file_monitor_source_handle_event(), but I’m not 100% sure that’s possible due to other checks beforehand.
Hm, I though this ticket was initially regarding Linux distros (there was an Ubuntu mentioned in the initial description), I doubt it was an Ubuntu based on BSD kernel, it's very rare... Thanks for the fix!
The bugs occur for me on Xubuntu 16.04 LTS, which is Linux. If this patch is only for BSD and OSX then it won't solve the problem on Linux. I do think it's strange that programs would use it if it was never available.
With all that said, I would revert subject back to what it was (unrelated to kqueue), and extend affected OSes list.
Right. Here’s an actual backtrace from a Linux system, retrieved from Launchpad: https://launchpadlibrarian.net/217269912/Stacktrace.txt. I’ll try and find time to take a look at this next week, but given that the inotify event details are not in the backtrace, any fix I come up with is going to be an educated guess.
+ Trace 238156
From that backtrace:
+ Trace 238184
I don’t think there’s much we can do to recover from events being dropped as a result of queue overflow. But we can at least not assert in that case. Patches coming shortly. I can’t think of a decent way of producing a reliable unit test for this, although I would really like to. inotify events are pulled off the inotify FD in a GLib worker thread, and we have no control over that worker thread — I can’t think of a way to suspend it, cancel and restart it, or otherwise cause it to not read the FD for long enough for a load of events to queue up. This is probably only replicable using a highly contested main context with a lot of inotify watches and events; and that can’t reliably be turned into a test. :-(
Created attachment 364356 [details] [review] kqueue: Fix invalid emission of G_FILE_MONITOR_EVENT_MOVED event That event is deprecated, and the kqueue backend can’t provide enough information to go alongside the event (i.e. the name of the new file). Use G_FILE_MONITOR_EVENT_DELETED instead. Quite disappointed in the kqueue documentation for this: I cannot find a single piece of documentation or example about how NOTE_RENAME is supposed to communicate the new name of the file. If it turns out that this is possible, the code can be amended again in future. At least now it doesn’t abort. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 364357 [details] [review] inotify: Ignore IN_Q_OVERFLOW events There’s not much we can do about them, and if they go unhandled, they can propagate through to g_file_monitor_source_handle_event() and cause assertion failures due to not mapping to a GFileMonitorEvent. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 364358 [details] [review] inotify: Don’t propagate unrecognised events to GLocalFileMonitor If we can’t convert the inotify event mask into a GFileMonitorEvent enum value, don’t propagate it to GLocalFileMonitor, since it hits an assertion failure in that case. This should no longer be possible since the previous commit to ignore IN_Q_OVERFLOW events, but we might as well change this just in case other bugs crop up in event mask handling. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 364356 [details] [review]: Looks reasonable.
Review of attachment 364357 [details] [review]: Okay
Review of attachment 364358 [details] [review]: Okay
Great, thanks for the reviews. Attachment 364356 [details] pushed as 76072a2 - kqueue: Fix invalid emission of G_FILE_MONITOR_EVENT_MOVED event Attachment 364357 [details] pushed as 9853842 - inotify: Ignore IN_Q_OVERFLOW events Attachment 364358 [details] pushed as 748bb24 - inotify: Don’t propagate unrecognised events to GLocalFileMonitor
Also backported to glib-2-54: 3246f1df9 inotify: Don’t propagate unrecognised events to GLocalFileMonitor d97e07709 inotify: Ignore IN_Q_OVERFLOW events 38ec9c6da kqueue: Fix invalid emission of G_FILE_MONITOR_EVENT_MOVED event
Non-backported fix is available in glib version 2.56.