GNOME Bugzilla – Bug 755721
g_inotify_file_monitor_start called with nullpointer for dirname and WATCH_HARD_LINKS causes a segfault
Last modified: 2018-04-18 14:44:45 UTC
I am currently getting the following segfault: (gdb) bt
+ Trace 235494
From the looks of it, it looks okay till frame #4 ie there is a filename and is_directory is 0. In frame #3 one might argue it gets weird since dirname/basename are null. When frame #2 is reached dirname and filename are both null, might be that the logic behind https://github.com/GNOME/glib/blob/master/gio/glocalfilemonitor.c#L646-L663 is flawed -- not sure. This is my first day ever looking at gio. Please tell me if you need more information (that said, I think the trace is perfectly clear and readable :))
Created attachment 312374 [details] [review] inotify: fix segfault on watching hard links The call to _start() fills in the dirname, basename, and filename arguments according to the following rules: dir watches: dirname filled file watfches: dirname and basename filled hardlink: filename filled This doesn't map to how the current inotify backend works very nicely, so we need to adjust things a bit when creating our "sub" objects.
Review of attachment 312374 [details] [review]: This should be fairly easy to add a test case for, right?
Good call. Wrote a test case and found out it just crashes further along anyway. I'll take another look at this tomorrow.
Seems like the fix is not yet in master, right? Just a gentle ping because hamster crashes for me without this fix...
No idea about whether this is committed or not, but upgrade hamster, they have a workaround as of https://github.com/projecthamster/hamster/commit/d869f7da0eec48df1f52771fed8ed12b12fbab61
hi. Still working on this -- sorry for the delay.
[Bumping TM from 3.20 to 3.26 though not sure this still happens]
(In reply to Allison Lortie (desrt) (extended vacation) from comment #3) > Good call. Wrote a test case and found out it just crashes further along > anyway. I'll take another look at this tomorrow. Do you want to attach that test case here, Allison? :-)
It also happens for me (I write some project with GFileMonitor. GLib version 2.54.3
Created attachment 368939 [details] [review] inotify: fix segfault on watching hard links The call to _start() fills in the dirname, basename, and filename arguments according to the following rules: dir watches: dirname filled file watches: dirname and basename filled hardlink: filename filled This doesn't map to how the current inotify backend works very nicely, so we need to adjust things a bit when creating our "sub" objects.
Created attachment 368940 [details] [review] inotify: Further fixes for hard link monitoring support This gets the G_FILE_MONITOR_WATCH_HARD_LINKS flag to the state where it doesn’t cause crashes, and essentially acts as a no-op. It will not yet actually monitor for changes made via hard links. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 368941 [details] [review] tests: Add a GFileMonitor test for G_FILE_MONITOR_WATCH_HARD_LINKS Add a test for monitoring an existing local file, with the WATCH_HARD_LINKS flag specified. This would previously cause a crash; now it doesn’t. This test contains a FIXME where I suspect we should be getting some additional file change notifications from changes made through the hard link; this requires further follow up and probably further fixes to our inotify backend. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Here’s an updated set of patches which fix the crash and add a unit test. The unit test makes it clear that the behaviour still isn’t right, I think, since we’re not receiving any change notifications for changes made to the monitored file via the hard link. That needs further follow-up, but I’d like to get these patches in for 2.56 to solve the crashes. We can then try and solve the missing notifications for 2.58.
Created attachment 368974 [details] [review] tests: Fix a minor memory leak in the GFileMonitor tests Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 368939 [details] [review]: Makes sense
Review of attachment 368940 [details] [review]: ::: gio/glocalfilemonitor.c @@ +235,3 @@ + { + gchar *dirname = g_path_get_dirname (fms->filename); + event->child = g_local_file_new_from_dirname_and_basename (dirname, child); You are leaking dirname here
Review of attachment 368941 [details] [review]: cool, a test
Review of attachment 368974 [details] [review]: sure
Review of attachment 368940 [details] [review]: ::: gio/glocalfilemonitor.c @@ +235,3 @@ + { + gchar *dirname = g_path_get_dirname (fms->filename); + event->child = g_local_file_new_from_dirname_and_basename (dirname, child); Oops, well caught, thanks.
Thanks. I fixed the leak (and another one a few lines further down) and have pushed to master. I will backport to glib-2-56 once the CI is happy, and will open a follow-up bug about fixing the FIXME in the test. Attachment 368939 [details] pushed as cc5cd5e - inotify: fix segfault on watching hard links Attachment 368940 [details] pushed as 3e4e005 - inotify: Further fixes for hard link monitoring support Attachment 368941 [details] pushed as 723ac89 - tests: Add a GFileMonitor test for G_FILE_MONITOR_WATCH_HARD_LINKS Attachment 368974 [details] pushed as d57f3e0 - tests: Fix a minor memory leak in the GFileMonitor tests
Follow-up is bug #795356.
CI passed (https://gitlab.gnome.org/GNOME/glib/-/jobs/25107), so I’ve backported it to glib-2-56 too: d060ab922 (HEAD -> glib-2-56, origin/glib-2-56) tests: Fix a minor memory leak in the GFileMonitor tests 21d761b4e tests: Add a GFileMonitor test for G_FILE_MONITOR_WATCH_HARD_LINKS b326c23d6 inotify: Further fixes for hard link monitoring support 3f66ab2ed inotify: fix segfault on watching hard links