Bug 21597

Summary: race condition in reloading config files
Product: dbus Reporter: Colin Walters <walters>
Component: coreAssignee: Havoc Pennington <hp>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: walters, zeuthen
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: a patch
updated patch
updated patch

Description Colin Walters 2009-05-06 08:22:33 UTC 
Right now the way config reloading works is that we have an inotify watch on the /etc/dbus-1/system.d directory, and when we detect one of these events (IN_CLOSE_WRITE | IN_DELETE | IN_MOVED_TO | IN_MOVED_FROM) we send SIGHUP to ourself, from which we write to a pipe, and that will cause the mainloop to wake up and trigger a reload.

I think basically the problem lies in the fact that we drop directory watches for a moment after a successful reload inside process_config_every_time, and then readd all the new watches in process_config_postinit.  That leaves a time window when we aren't monitoring for inotify events.  Leading to a situation like the following when installing a package update where multiple config files are being written out in quick succession (not uncommon at all, NetworkManager comes with several in one package).

* rpm/dpkg begins unpacking each file into /etc/dbus-1/system.d
* rpm/dpkg completes the first file
* dbus is scheduled gets inotify event IN_CLOSE_WRITE, gets scheduled and begins reading config files
* dbus successfully rereads all config files
* dbus drops all inotify watches
* rpm/dpkg starts writes out the second file
* dbus readds all directory watches

This whole thing is complicated enough that I'm not convinced there aren't other races, but this seems like at least one.
Comment 1 Colin Walters 2009-05-06 08:29:08 UTC 
One solution that comes to mind is to only do the drop/readd if the set of monitored directories is different (and in reality, it basically never will be)

Another thought - a "big crude hammer" fix might be to:

while (config_changed) {
  if (parse_config() == SUCCESS) {
    config_changed = FALSE;
    break;
  }
  sleep(1);
}
  
Additional data here is a sample sequence of events for dpkg installing an updated config file:
http://paste.ubuntu.com/165526/
Comment 2 Matthias Clasen 2009-12-17 22:00:26 UTC 
Created attachment 32166 [details] [review]
a patch

Here is a patch that fixes this for the inotify implementation by replacing bus_watch_directory and bus_drop_all_directory_watches by bus_set_watched_dirs which takes care not to drop any directories that should remain watched.
Comment 3 Colin Walters 2010-01-28 13:37:34 UTC 
Created attachment 32881 [details] [review]
updated patch

In the big picture this patch really obsoletes the old monitoring API; the old functions were no longer called by the bus core.

So I deleted them from dir-watch-inotify.c.

I did some other small changes like documenting the API, fixing mismatch of strdup and dbus_free.
Comment 4 Colin Walters 2010-01-28 13:55:35 UTC 
Created attachment 32882 [details] [review]
updated patch

Register a shutdown func to clean up the dbus_strdup memory, otherwise test suite hates us.
Comment 5 Colin Walters 2010-02-02 07:35:30 UTC 
Pushed to dbus-1.2 as 8a9880ffd2

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.