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

[Quentin Glidic]

On 09/06/2016 12:05, Pekka Paalanen wrote:
> 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?

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