On 09/06/2016 12:05, Pekka Paalanen wrote:
On Sun, 29 May 2016 13:59:14 +0200
Quentin Glidic <[email protected]> wrote:
From: Quentin Glidic <[email protected]>
This way, the environment is correctly preserved for weston. Since
commit 636156d5f693ac5b01cec6a2937d2b6cd4237ea9, clearenv() is only
called when we open a new PAM session, so it makes sense to only use a
login shell in that case.
Signed-off-by: Quentin Glidic <[email protected]>
---
src/weston-launch.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/src/weston-launch.c b/src/weston-launch.c
index b8dfb17..d731ed8 100644
--- a/src/weston-launch.c
+++ b/src/weston-launch.c
@@ -577,8 +577,8 @@ setup_tty(struct weston_launch *wl, const char *tty)
return 0;
}
-static void
-setup_session(struct weston_launch *wl)
+static int
+setup_session(struct weston_launch *wl, char *child_argv[MAX_ARGV_SIZE])
Hi,
that is an array to char-pointer as a function parameter. I never pass
arrays as parameters like that, so can you explain, does it correspond
to char** or is the function operating on a copy of the array which
would be wrong?
Honestly, I had to write a test program to check that, and it’s not
copied. Just changed it to char** to avoid the confusion.
Because I even have to ask the question for this setuid-root program is
an indication that it would probably be better formulated so that I
don't even have to think about it.
{
char **env;
char *term;
@@ -608,6 +608,17 @@ setup_session(struct weston_launch *wl)
}
free(env);
}
+
+ /*
+ * We open a new session, so it makes sense
+ * to run a new login shell
+ */
+ child_argv[0] = "/bin/sh";
+ child_argv[1] = "-l";
+ child_argv[2] = "-c";
+ child_argv[3] = BINDIR "/weston \"$@\"";
+ child_argv[4] = "weston";
+ return 5;
}
static void
@@ -626,12 +637,19 @@ launch_compositor(struct weston_launch *wl, int argc,
char *argv[])
{
char *child_argv[MAX_ARGV_SIZE];
sigset_t mask;
- int i;
+ int o, i;
if (wl->verbose)
printf("weston-launch: spawned weston with pid: %d\n",
getpid());
if (wl->new_user)
Braces needed now because the else has them.
Fixed.
- setup_session(wl);
+ o = setup_session(wl, child_argv);
+ else {
+ child_argv[0] = BINDIR "/weston";
+ o = 1;
+ }
+ for (i = 0; i < argc; ++i)
+ child_argv[o + i] = argv[i];
+ child_argv[o + i] = NULL;
if (geteuid() == 0)
drop_privileges(wl);
@@ -648,14 +666,6 @@ launch_compositor(struct weston_launch *wl, int argc, char
*argv[])
sigaddset(&mask, SIGINT);
sigprocmask(SIG_UNBLOCK, &mask, NULL);
- child_argv[0] = "/bin/sh";
- child_argv[1] = "-l";
- child_argv[2] = "-c";
- child_argv[3] = BINDIR "/weston \"$@\"";
- child_argv[4] = "weston";
- for (i = 0; i < argc; ++i)
- child_argv[5 + i] = argv[i];
- child_argv[5 + i] = NULL;
execv(child_argv[0], child_argv);
error(1, errno, "exec failed");
The change looks good to me in principle, but we need two R-bs and
would like a Tested-by for both paths: with and without new_user.
My R-b is pending on the two issues mentioned above.
You could list the ways you tested it with in the commit message.
Nothing fancy here, just checked an obvious variable ($PATH) with and
without -u after the patch.
Thanks,
pq
I resend a new series.
Cheers,
--
Quentin “Sardem FF7” Glidic
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
