|Summary:||Session dbus-daemon should not change umask|
|Product:||dbus||Reporter:||Matt McCutchen <matt>|
|Component:||core||Assignee:||Havoc Pennington <hp>|
|Status:||RESOLVED FIXED||QA Contact:||John (J5) Palmieri <johnp>|
|i915 platform:||i915 features:|
Oops, correct bug number in patch
Revised patch with keep_umask option
Description Matt McCutchen 2008-11-08 12:31:55 UTC
dbus-daemon changes its umask to 0022. gnome-settings-daemon and Firefox processes that I start with the /apps/gnome_settings_daemon/keybindings/www shortcut key are inheriting this umask, which makes Firefox save downloaded files with the wrong permissions. I think dbus-daemon should not change its umask.
Comment 2 Matt McCutchen 2008-11-08 12:38:30 UTC
Created attachment 20156 [details] [review] Oops, correct bug number in patch
Comment 3 Havoc Pennington 2008-11-08 16:31:16 UTC
If applying this, it's not necessary to leave an archeology comment saying what code used to be there, just remove the old code with a ChangeLog entry. That said, this is a potentially dangerous change, and would be ill-advised without at minimum reviewing all places where dbus creates files, and evaluating how this could impact them. Generally daemons set umask to avoid surprises, because umask is well-known to cause surprises. Most daemons do this. A safer change might be to record the old umask (returned from the umask call), and when we spawn child processes, set the old umask in the child. That way dbus-daemon itself is never affected by the system environment and always gets the permissions it expects.
Comment 4 Matt McCutchen 2008-11-08 17:52:03 UTC
(In reply to comment #3) > That said, this is a potentially dangerous change, and would be ill-advised > without at minimum reviewing all places where dbus creates files, and > evaluating how this could impact them. Generally daemons set umask to avoid > surprises, because umask is well-known to cause surprises. Most daemons do > this. I was just thinking about the per-user daemon and had forgotten that there's a system daemon too. In my view, setting the umask makes perfect sense for the system daemon since it is in control of the security model and knows who it wants to be able to access its files. On the other hand, I don't see why a per-user daemon should override the umask that I have established for all the other processes in my session. I ran the following to find all the places dbus creates files: git grep -A 1 -E '\<(O_CREAT\>|(mkdir|symlink|mknod|bind) *\()' And it appears that there is no created file whose permissions would become looser as a result of not setting the umask. I'm guessing that permissions becoming tighter is not a problem because the daemon is single-user. > A safer change might be to record the old umask (returned from the umask call), > and when we spawn child processes, set the old umask in the child. That way > dbus-daemon itself is never affected by the system environment and always gets > the permissions it expects. That would also fix my problem, but please consider whether it would not make sense to drop the umask setting altogether for a per-user daemon.
Comment 5 Havoc Pennington 2008-11-08 18:14:18 UTC
Agreed, could make sense to drop it for the session daemon. To do this, we'd have to add an option to the config file like <keep_umask> and set it in session.conf.
Comment 6 Matt McCutchen 2008-11-08 18:16:42 UTC
I was about to hard-code dropping the umask for a session daemon but not a system daemon. I will implement the config option instead.
Comment 7 Matt McCutchen 2008-11-08 19:37:44 UTC
Created attachment 20160 [details] [review] Revised patch with keep_umask option That was an awful lot of work to add one boolean!
Comment 8 Havoc Pennington 2008-11-09 11:55:14 UTC
Patch looks great to me. Can someone merge this? (I think I still have ssh key problems with freedesktop.org git)