It's easy enough to fix the execv(), but we need to call PAM with the new environment already set up, and that's after all D-Bus interaction.

So we might need to do the PAM etc. in a non-threaded process, either using fork() or execve() to "get rid of" the other threads (the second being much safer).

Somewhat related, I'm more and more convinced that PAM should get a "run $this in a newly created user's session" function that hides a lot of this (e.g. the pam_getenvlist handling) from the calling application entirely.

Does this actually happen in practice in the pkexec(1) program? I don't disagree this can happen (especially in more interesting programs than pkexec), but I doubt this is something we need to actually worry about in pkexec itself.

FWIW, I'd also rather GLib not (indirectly or otherwise) call getenv() in any of its private worker threads. I think that's the real problem and needs fixing - GLib's worker threads - at least the GDBus one - should basically be invisible.

(In reply to comment #2)

> I don't
> disagree this can happen (especially in more interesting programs than
> pkexec), but I doubt this is something we need to actually worry about in
> pkexec itself.

Probably true, but if we add a g_warning() to GLib as we'd like to do, then pkexec would emit a warning.

> FWIW, I'd also rather GLib not (indirectly or otherwise) call getenv() in
> any of its private worker threads. I think that's the real problem and needs
> fixing - GLib's worker threads - at least the GDBus one - should basically
> be invisible.

Yeah...really a lot of pain just for the code that lives in git.gnome.org/glib if we tried to keep translated error messages.  Maybe if glib*c* adds getenv_r() and changes gettext to use it we could consider porting GLib, but at the high level view it's just a lot easier to fix the (relatively few) apps that call setenv() after early startup.

See http://sourceware.org/bugzilla/show_bug.cgi?id=15607 
There's little hope of getting a getenv_r and having gettext use it.

> Probably true, but if we add a g_warning() to GLib as we'd like to do,
> then pkexec would emit a warning.

Please don't. It's only going to alienate people using GLib. Seriously, the whole GLib/GTK+ stack is causing enough grief as is.

> > Probably true, but if we add a g_warning() to GLib as we'd like to do,
> > then pkexec would emit a warning.
> 
> Please don't. It's only going to alienate people using GLib. Seriously, the 
> whole GLib/GTK+ stack is causing enough grief as is.

Huh? We want to make g_setenv() generate a warning when a worker thread has been started. That's completely legitimate.

GLib not using gettext (for example) in worker threads is absolutely unworkable. Want to use GIO-style async with a translated error message? Nope, sorry, can't do that.

What I'm saying is that it's completely bonkers if a program can't use g_setenv() anymore just because the GLib authors (this would include myself) implements their library in non-optimal ways.

For sure there are ways around this, such as e.g. delaying e.g. gettext() calls until you pop the result/error off the worker thread... aka in the _finish() methods called from a thread under the control of the user.

What is _not_ OK is telling developers "oh, sorry, you can't use setenv(3) or g_setenv() anymore because we made GLib more awesome". That's simply not going to make developers think GLib is more awesome. They'll just think "meh" and use something else. Seriously.

(In reply to comment #7)

> For sure there are ways around this, such as e.g. delaying e.g. gettext()
> calls until you pop the result/error off the worker thread... aka in the
> _finish() methods called from a thread under the control of the user.

It's really just not that easy...

> What is _not_ OK is telling developers "oh, sorry, you can't use setenv(3)
> or g_setenv() anymore 

You can still use setenv() - you just have to do it before using GLib (or any library that might create threads that could call getenv()).

> because we made GLib more awesome".

This isn't a new issue at all; we've been seeing random crashes due to this for years.

>  That's simply not
> going to make developers think GLib is more awesome. They'll just think
> "meh" and use something else. Seriously.

For some projects, not using GLib may make sense actually.  In this specific case of pkexec, I don't like the idea of supporting setuid binaries using GLib myself (particularly not ones using Gio).

But for a lot of other projects, the path of least resistance really is to adjust to calling setenv() early.

David, your rethoric is misplaced here. This is not about glib at all. If you use posix setenv() and gettext() in a threaded program, you have all the same issues.

(In reply to comment #9)
> David, your rethoric is misplaced here. This is not about glib at all. If
> you use posix setenv() and gettext() in a threaded program, you have all the
> same issues.

No, Matthias, you are wrong. If GLib starts spamming stderr (using g_warning()) if a program is using g_setenv(), that's a problem. Because, for all GLib knows, the program could have been careful and using g_setenv() behind a lock or from only a single thread or whatever. The fact that GLib is using g_setenv() from its private worker threads is a problem (and a fixable one, at that).

(In reply to comment #10)
> The fact that GLib
> is using g_setenv() from its private worker threads is a problem (and a
> fixable one, at that).

We're not calling setenv from worker threads, only *get*env().  

(Well, ok your gdbus code does call setenv, but only if a developer environment variable is set, and it'd be pretty easy to fix)

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/polkit/polkit/issues/15.