[PATCH weston 1/2] weston-launch: Only run a login shell for new sessions

[Pekka Paalanen]

On Sun, 29 May 2016 13:59:14 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

> From: Quentin Glidic <sardemff7+git at sardemff7.net>
> 
> 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 <sardemff7+git at sardemff7.net>
> ---
>  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160609/29320189/attachment.sig>