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? 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. > - 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. Thanks, pq
pgp4BqUmcBqJf.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
