GNOME Bugzilla – Bug 408309
Handling of the renaming of gdm socket is a bit ugly
Last modified: 2008-10-27 15:14:17 UTC
Please describe the problem: The problem is described in bug #84944 on ubuntu's launchpad. See https://bugs.launchpad.net/ubuntu/+source/fast-user-switch-applet/+bug/84944 I also describe a workaround there. A fix, however, would be to set GDM_SOCKET_FILENAME in src/gdm-queue.c:46 according to GDM version in use. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 83195 [details] [review] Possible fix Here's a possible fix. If nobody complains, I'll commit it tonight.
Going in for tonight's release, then.
Thomas: I don't think the patch is good since the file change is ubuntu-specific. While it's probably okay to allow the use of two files, the name (OLD and NEW) is not good.
Also consider the patch in bug #407524 comment 14. Note that I've removed all of the FUSA code from gnome-screensaver but you may want this fix too.
Checking two places for a socket may also be make FUSA somewhat more vunerable to attack if something else creates a socket in /tmp. Perhaps you should check the ownership of the socket or something?
(In reply to comment #3) > Thomas: I don't think the patch is good since the file change is > ubuntu-specific. While it's probably okay to allow the use of two files, the > name (OLD and NEW) is not good. Vincent, I think the Debian/Ubuntu change makes sense and should be done upstream in gdm too (see bug 331059); there might even be some FHS and security incentives to change this. It seem the patch has been checked in already, so I'm renaming this bug to reflect that the change is a bit ugly because: - it uses stat() instead of g_file_test() - it uses the concept of "NEW" and "OLD" Anything else?
(In reply to comment #5) > Checking two places for a socket may also be make FUSA somewhat more vunerable > to attack if something else creates a socket in /tmp. Perhaps you should check > the ownership of the socket or something? Do you know whether the ownership of the socket is guaranteed? On my system, it's root:root, but I wonder whether it could be gdm:something. I do agree using a socket in /tmp blindly has security implications, but I think we're bound to check for it for backwards compatibility...
Created attachment 87947 [details] [review] Parse a GDM_SOCKET_PATH list of sockets; test existence with g_file_test instead of stat()
@William, let me know if a test for uid/gid 0/0 makes sense. (BTW I only built the above patch but did not run it.) @Vincent, is this clean enough?
Created attachment 92294 [details] [review] Patch to correct error in committed patch This patch was committed and breaks things in 2.19.x if (stat (GDM_NEW_SOCKET_FILENAME, &file_stat) == 0) strcpy (addr.sun_path, GDM_NEW_SOCKET_FILENAME); if (stat (GDM_OLD_SOCKET_FILENAME, &file_stat) == 0) strcpy (addr.sun_path, GDM_OLD_SOCKET_FILENAME); else g_error ("Can't find gdm socket!"); Means that if there is a new socket but no old socket we throw a g_error which aborts. Flow control probably needs to be if/else if/else, this worked downstream
Right; and this is also solved by the alternate implementation which I proposed in response to the critic that "NEW" and "OLD" wasn't very nice.
*** Bug 412541 has been marked as a duplicate of this bug. ***
JPR's bug is identical to the patch given in bug 412541 comment 2, and I have just committed it.
Loïc: you rock a whole lot. Your patch is a much better idea, and I have committed it with a few minor changes: 1) a couple of comments 2) if you treat a pointer as a boolean at one point, there's no reason to explicitly compare it against NULL later on. Thanks a ton!
*** Bug 485158 has been marked as a duplicate of this bug. ***
*** Bug 510073 has been marked as a duplicate of this bug. ***
*** Bug 556792 has been marked as a duplicate of this bug. ***
*** Bug 555541 has been marked as a duplicate of this bug. ***