GNOME Bugzilla – Bug 778422
gsubprocesslauncher: Clarify the behavior of set_environ()
Last modified: 2017-05-31 11:08:06 UTC
Calling g_subprocess_launcher_set_environ with NULL doesn't prevent the subprocess from inheriting its parent's env vars, so I think we should either clarify the behavior or change the behavior in gspawn.c, but doing the latter might cause problems for projects depending on the current behavior.
Created attachment 345393 [details] [review] gsubprocesslauncher: Clarify the behavior of set_environ() GNOME Builder's code was assuming that setting the launcher's environ to NULL makes the subprocess have an empty environment, but in fact the parent process's variables are still inherited because execv is used instead of execve when envp is NULL. This commit clarifies the documentation to make the behavior clear.
Review of attachment 345393 [details] [review]: This is phrased as if passing NULL and getting the parent’s environment is a bug — I don’t think it necessarily is. I’d phrase the documentation more neutrally, for example: “Pass NULL to inherit the parent process’ environment. Pass an empty array to set an empty environment.” What might be a bug here is that calling g_subprocess_launcher_set_environ(l, NULL) then g_subprocess_launcher_unsetenv(l, "some-variable-set-in-parent-environment") still results in that variable being set in the child, as the parent process’ environment is resolved when g_spawn_async() is called; not when set_environ(NULL) is called. That could be fixed in set_environ(), but would be a minor API change for this sequence of calls. Going to need a second opinion on that. ::: gio/gsubprocesslauncher.c @@ +242,3 @@ * + * Note that setting this to NULL does not prevent the subprocess from + * inheriting the parent proccess's environment variables. For that, s/proccess's/process’/ (typo)
Created attachment 345468 [details] [review] gsubprocesslauncher: Clarify the behavior of set_environ() GNOME Builder's code was assuming that setting the launcher's environ to NULL makes the subprocess have an empty environment, but in fact the parent process's variables are still inherited because execv is used instead of execve when envp is NULL. This commit clarifies the documentation to make the behavior clear.
Review of attachment 345468 [details] [review]: Looks good to me, thanks.
Pushed as commit 442d64ba94d8ddae24a79d1a0eaab1e543804163. Not sure if we should leave the bug open for the other issue mentioned by Philip
(In reply to Philip Withnall from comment #2) > What might be a bug here is that calling > g_subprocess_launcher_set_environ(l, NULL) then > g_subprocess_launcher_unsetenv(l, "some-variable-set-in-parent-environment") > still results in that variable being set in the child, as the parent > process’ environment is resolved when g_spawn_async() is called; not when > set_environ(NULL) is called. That could be fixed in set_environ(), but would > be a minor API change for this sequence of calls. Going to need a second > opinion on that. Colin, do you have any thoughts on this?
(In reply to Philip Withnall from comment #2) > What might be a bug here is that calling > g_subprocess_launcher_set_environ(l, NULL) then > g_subprocess_launcher_unsetenv(l, "some-variable-set-in-parent-environment") > still results in that variable being set in the child, as the parent > process’ environment is resolved when g_spawn_async() is called; not when > set_environ(NULL) is called. That could be fixed in set_environ(), but would > be a minor API change for this sequence of calls. Going to need a second > opinion on that. We call g_get_environ() in _init() so it's at least slightly clear here that we intended to persist a separate copy of the parent's environment in the launcher itself. Calling _set_environ() with NULL changes that assumption, which is weird in my opinion. imho, we should make it so that _set_environ(NULL) copies the parent environment at that point, and document that clearly. That's a break, but it's a really really minor one, and we already have a vague rule that it's unsafe to setenv() after starting to use GLib anyway, so this is just another reason not to do that.
Created attachment 348983 [details] [review] gsubprocess: Copy parent process’ environ when clearing subprocess’ Previously, this was done at the time of spawning the subprocess, which meant the g_subprocess_launcher_*_environ() functions could not be used to modify the parent process’ environment. Change the code to copy the parent process’ environment when g_subprocess_launcher_set_environ(NULL) is called. Document the change and add a unit test.
Review of attachment 348983 [details] [review]: Looks good to me. I agree that we should make this change.
Thanks for the review! Attachment 348983 [details] pushed as e1e73da - gsubprocess: Copy parent process’ environ when clearing subprocess’