Various pam modules provide environment variables that are intended to be set in the environment of the pam session. pkexec needs to process the output of pam_getenvlist() to get these. This will e. g. apply correct locales in pkexec when they are configured in pam_environment.
Created attachment 76150 [details] [review] pkexec: Set process environment from pam_getenvlist() Steve Langasek applied this patch a while ago to the Ubuntu packages. I adjusted it for current git master and brought it into git format-patch form.
I'm not sure that's a good idea ... but I can probably be convinced that it is :-) ... So apart from locales, can you give examples of such PAM modules and the environment variables that are set? Thanks.
Comment on attachment 76150 [details] [review] pkexec: Set process environment from pam_getenvlist() Review of attachment 76150 [details] [review]: ----------------------------------------------------------------- Looks good but the the coding style is wrong - curly-braces / indentation wrong - should use guint instead of int - should use 'n' as a counter/iterator, not 'i' (like the rest of the code) These are style issues but consistency is important.
On Fri, Mar 08, 2013 at 06:06:28PM +0000, bugzilla-daemon@freedesktop.org wrote: > ... So apart from locales, can you give examples of such PAM modules and > the environment variables that are set? Thanks. The pam_env module is a big one, which is used by admins to configure arbitrary environment settings for all sessions. The specific case that prompted this had to do with proxy settings configured in the environment. Other modules that may need to set environment variables include pam_krb5 and pam_afs_session, whose environment settings may be required for proper filesystem access.
Created attachment 76324 [details] [review] pkexec: Set process environment from pam_getenvlist() Fixed coding style.
Comment on attachment 76324 [details] [review] pkexec: Set process environment from pam_getenvlist() Review of attachment 76324 [details] [review]: ----------------------------------------------------------------- ::: src/programs/pkexec.c @@ +182,5 @@ > + { > + guint n; > + for (n = 0; envlist[n]; n++) > + putenv (envlist[n]); > + free (envlist); From my reading of the manual page (haven't looked at the source), it looks to me like you need to free() the individual elements too. I.e., do what g_strfreev() does. Right? (Just looked at the source of Linux-PAM-1.1.6, and that does indeed appear to be the case)
> From my reading of the manual page (haven't looked at the source), > it looks to me like you need to free() the individual elements too. Not according to the manpage for putenv(), which states that the string passed to putenv() becomes part of the environment directly ("The string pointed to by string becomes part of the environment, so altering the string changes the environment").
(In reply to comment #7) > > From my reading of the manual page (haven't looked at the source), > > it looks to me like you need to free() the individual elements too. > > Not according to the manpage for putenv(), which states that the string > passed to putenv() becomes part of the environment directly ("The string > pointed to by string becomes part of the environment, so altering the string > changes the environment"). Sorry about the slow response; I subscribed to drm-devel@ recently and that ended up in a total flood of my bugzilla-daemon@freedesktop.org mail. So anyways, yes looks like you're right. The environment APIs are just ugly. I just looked at the GDM source, and it appears to call the same API, so this makes sense. Pushed to master, thanks!
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.