After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779607 - Race between mounts-changed signal and g_unix_mounts_get() function
Race between mounts-changed signal and g_unix_mounts_get() function
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.50.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-03-05 11:18 UTC by Chris Vine
Modified: 2017-08-03 08:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test case (846 bytes, text/plain)
2017-03-05 11:21 UTC, Chris Vine
  Details
gunixmounts: Prevent "mounts-changed" race if /etc/mtab is used (1.47 KB, patch)
2017-07-17 14:44 UTC, Ondrej Holy
committed Details | Review
gunixmounts: Add missing const qualifier for mtab path (2.45 KB, patch)
2017-08-03 08:23 UTC, Ondrej Holy
committed Details | Review

Description Chris Vine 2017-03-05 11:18:50 UTC
In glib-2.50, there is a race between the mounts-changed signal emitted by GUnixMountMonitor objects on the one hand and g_unix_mounts_get() on the other hand, which means that the handler of the mounts-changed signal may get a stale list of mount points from g_unix_mounts_get(): that is, one which does not include the change which caused the signal to be emitted in the first place.

There are weirdities about this.  On my system (running slackware-current), this race only manifests itself clearly when the init which is running is SysV init.  When the init is systemd the race does not appear.

This race appears first to have arisen in glib-2.50.1 (the mounts-changed signal did not work at all with glib-2.50.0 - see fixed bug #772092).  I will attach a test case which demonstrates the problem.
Comment 1 Chris Vine 2017-03-05 11:21:50 UTC
Created attachment 347261 [details]
Test case

Attached is a test case which shows the problem.  On my system, for the problem to arise an init other than systemd must be on PID 1.
Comment 2 Chris Vine 2017-06-10 10:39:13 UTC
Looking further at this, the problem seemed to arise at glib-2.44.  The reason why behavior differed between a computer running SysV init and one running systemd is because systemd linked /etc/mtab to /proc/mounts, whereas the computer running SysV init used mount to generate /etc/mtab.

So the result is that with recent versions of glib, the handler of the mounts-changed signal can only obtain useful mount information on systems running linux if /etc/mtab is linked to /proc/mounts or /proc/self/mounts.
Comment 3 Philip Withnall 2017-06-12 12:31:48 UTC
(In reply to Chris Vine from comment #2)
> Looking further at this, the problem seemed to arise at glib-2.44.

It’s probably related to bug #522053 then.

What exactly are the differences in the data returned by mounts-changed and g_unix_mounts_get() for you? How do they relate to the contents of /etc/mtab and /proc{,/self}/mounts? I don’t have a non-systemd system to test this with.
Comment 4 Chris Vine 2017-06-12 21:03:21 UTC
> What exactly are the differences in the data returned by mounts-changed
> and g_unix_mounts_get() for you?

The mounts-changed signal does provide any mount data.  The signal handler is expected to call g_unix_mounts_get() to obtain the current list of mounts.

The problem is that unless /etc/mtab is a link to /proc/mounts or /proc/self/mounts, the signal handler when calling g_unix_mounts_get() obtains a stale list of mounts, which does not include the mount change which caused the signal to be emitted in the first place.  This makes the mounts-changed signal kind of useless for cases where /etc/mtab is derived from mount and not /proc/mounts.

> How do they relate to the contents of /etc/mtab and /proc{,/self}/mounts?

I have no idea how g_unix_mounts_get() is related to the contents of /etc/mtab.  The bug to which you referred suggests that that function obtains its information from libmount (where libmount is installed, which is the case with the computers in question).

So maybe the bug only arises where /etc/mtab is not linked to /proc/mounts AND libmount is installed.  Dunno.
Comment 5 Chris Vine 2017-06-12 21:07:16 UTC
Err,

> The mounts-changed signal does provide any mount data
  
The mounts-changed signal does _not_ provide any mount data
Comment 6 Philip Withnall 2017-07-04 10:35:56 UTC
Ondrej, do you think this one could also be a problem caused by not running the main loop, as with bug #759644?
Comment 7 Ondrej Holy 2017-07-10 10:23:14 UTC
(In reply to Philip Withnall from comment #6)
> Ondrej, do you think this one could also be a problem caused by not running
> the main loop, as with bug #759644?

Nope, this is definitely something else. As far as I can tell, caching is not used here, g_unix_mounts_get always read fresh data and don't need the main loop at all. 

It seems that different file is monitored than it is really used by g_unix_mounts_get, or libmount caches the data internally somehow. Maybe we should use libmnt_monitor instead of custom monitoring...

Karel, don't you have a clue what needs to be done here?
Comment 8 Ondrej Holy 2017-07-17 14:44:32 UTC
Created attachment 355755 [details] [review]
gunixmounts: Prevent "mounts-changed" race if /etc/mtab is used

The /etc/mtab file is still used by some distributions (e.g. Slackware),
so it has to be monitored instead of /proc/self/mountinfo in order to
avoid races between g_unix_mounts_get and "mounts-changed" signal. The
util-linux is built with --enable-libmount-support-mtab in that case and
mnt_has_regular_mtab is used for checks. Let's use mnt_has_regular_mtab
also to determine which file to monitor.
Comment 9 Ondrej Holy 2017-07-17 14:45:33 UTC
Chris, can you please test the proposed patch?
Comment 10 Chris Vine 2017-07-18 09:22:25 UTC
The proposed patch works on my current computer when it uses a standalone /etc/mtab file, and with /etc/mtab linked to /proc/mounts.  I would like to test further on a different computer on Thursday and I will report back again.
Comment 11 Chris Vine 2017-07-20 14:00:14 UTC
I have now tested the patch on three machines with varying combinations of systemd and SysV init, and of freestanding /etc/mtab and of /etc/mtab linked to /proc/mounts.  The patch seems to fix the bug concerned.
Comment 12 Philip Withnall 2017-07-28 09:57:08 UTC
Review of attachment 355755 [details] [review]:

Looks good to me, thanks for the patch.

Relatedly, the return type for get_mtab_monitor_file() should be `const`. Could you push a separate commit to change that please Ondrej?
Comment 13 Philip Withnall 2017-07-28 09:57:30 UTC
Also, thanks for testing the patch, Chris!
Comment 14 Ondrej Holy 2017-08-03 08:23:17 UTC
Created attachment 356829 [details] [review]
gunixmounts: Add missing const qualifier for mtab path

get_mtab_read_file and get_mtab_monitor_file returns const path,
but const qualifier is missing. Let's add it.
Comment 15 Ondrej Holy 2017-08-03 08:24:19 UTC
Thanks all for reviews and testing!

Attachment 355755 [details] pushed as 2db36d0 - gunixmounts: Prevent "mounts-changed" race if /etc/mtab is used
Attachment 356829 [details] pushed as 41a4a70 - gunixmounts: Add missing const qualifier for mtab path
Comment 16 Ondrej Holy 2017-08-03 08:40:56 UTC
Pushed to glib-2-52 and glib-2-50 as well.