GNOME Bugzilla – Bug 687920
GCredentials should have an accessor for the process ID
Last modified: 2012-12-19 20:14:25 UTC
gdm has: static GPid credentials_get_unix_pid (GCredentials *credentials) { GPid pid = 0; gpointer native_credentials = NULL; #ifdef __linux__ native_credentials = g_credentials_get_native (credentials, G_CREDENTIALS_TYPE_LINUX_UCRED); pid = (GPid) ((struct ucred *) native_credentials)->pid; (etc.) I can't help thinking this shouldn't be necessary: there should be a g_credentials_get_unix_pid() or something. (By inspection, the implementation in gdm also appears to be untested and wrong on FreeBSD, for which I'll open a separate bug.)
See Bug #687921 for ways in which the implementation in gdm is wrong on (k)FreeBSD.
Created attachment 228469 [details] [review] [1/3] GCredentials: add getter/setter for the Unix process ID --- Opportunities for bikeshedding: * pid, process or process_id? I chose pid - if you don't know what that means, this function is not meant for you. * Return pid_t or GPid? I chose pid_t, which can be bigger than GPid (Bug #399148). Only tested on Linux, but the regression tests I'm about to add will verify whether it works on FreeBSD, kFreeBSD and OpenBSD.
Created attachment 228470 [details] [review] [2/3] GCredentials: add a regression test
Created attachment 228471 [details] [review] [3/3] Check that credentials pass through D-Bus on supported platforms --- We (claim to) support Linux, FreeBSD, kFreeBSD and OpenBSD, of which I have only tested Linux. (I think libdbus can also do this on Solaris, if anyone cares.)
We'd talked about this in the original GCredentials bug (bug 687920) but it never happened... I guess because David didn't need it right then. There doesn't seem to have been any explicit decision to not do it. Do you have a use case for g_credentials_set_pid()? _set_unix_user() is for the case where root wants to spoof as a different user. I doubt any platforms let you spoof the pid though...
(In reply to comment #5) > We'd talked about this in the original GCredentials bug (bug 687920) but it > never happened... I guess because David didn't need it right then. There > doesn't seem to have been any explicit decision to not do it. Right, my recollection is that agreed that it'd be fine to add things like this in the future and when there's a real need. I think it would be fine to add this if it helps application and library authors (and I think it will). > Do you have a use case for g_credentials_set_pid()? _set_unix_user() is for the > case where root wants to spoof as a different user. I doubt any platforms let > you spoof the pid though...
(In reply to comment #5) > Do you have a use case for g_credentials_set_pid()? No, I wouldn't mind removing it; I only added it out of symmetry. If that's the use-case for set_unix_user(), then, yes, set_pid() makes little sense. As far as I'm concerned, the important thing is getting the platform-specific goo out of (e.g.) gdm, and into somewhere where people on exotic platforms might actually test it, if only via "make check" :-)
Review of attachment 228469 [details] [review]: need to add g_credentials_get_unix_pid() to docs/reference/gio/gio-sections.txt ::: gio/gcredentials.c @@ +559,3 @@ + */ +gboolean +g_credentials_set_unix_pid (GCredentials *credentials, as discussed, kill this (here, in the header, and in gio.symbols) ::: gio/gcredentials.h @@ +67,3 @@ uid_t g_credentials_get_unix_user (GCredentials *credentials, GError **error); +pid_t g_credentials_get_unix_pid (GCredentials *credentials, need to tag this GLIB_AVAILABLE_IN_2_36
Review of attachment 228470 [details] [review]: looks good (other than the set_pid stuff)
Review of attachment 228471 [details] [review]: makes sense