[PATCH weston 3/3] weston-terminal: Fix race at startup

[Quentin Glidic]

On 3/24/17 3:41 PM, Derek Foreman wrote:
> If anything is printed for the terminal window to display before the
> window has been initially sized we end up with a segfault.
> 
> This defers the exec() of the shell child process until after the
> window is sized so this can't happen anymore.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>   clients/terminal.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/clients/terminal.c b/clients/terminal.c
> index c5531790..1060c58b 100644
> --- a/clients/terminal.c
> +++ b/clients/terminal.c
> @@ -38,6 +38,7 @@
>   #include <sys/epoll.h>
>   #include <wchar.h>
>   #include <locale.h>
> +#include <errno.h>
>   
>   #include <linux/input.h>
>   
> @@ -481,6 +482,7 @@ struct terminal {
>   	int selection_start_row, selection_start_col;
>   	int selection_end_row, selection_end_col;
>   	struct wl_list link;
> +	int pace_pipe;
>   };
>   
>   /* Create default tab stops, every 8 characters */
> @@ -860,6 +862,10 @@ resize_handler(struct widget *widget,
>   	struct terminal *terminal = data;
>   	int32_t columns, rows, m;
>   
> +	if (terminal->pace_pipe >= 0) {
> +		close(terminal->pace_pipe);
> +		terminal->pace_pipe = -1;
> +	}
>   	m = 2 * terminal->margin;
>   	columns = (width - m) / (int32_t) terminal->average_width;
>   	rows = (height - m) / (int32_t) terminal->extents.height;
> @@ -3027,9 +3033,33 @@ terminal_run(struct terminal *terminal, const char *path)
>   {
>   	int master;
>   	pid_t pid;
> +	int pipes[2];
> +
> +	/* Awkwardness: There's a sticky race condition here.  If
> +	 * anything prints after the forkpty() but before the window has
> +	 * a size then we'll segfault.  So we make a pipe and wait on
> +	 * it before actually exec()ing the terminal.  The resize
> +	 * handler closes it in the parent process and the child continues
> +	 * on to launch a shell.
> +	 *
> +	 * The reason we don't just do terminal_run() after the window
> +	 * has a size is that we'd prefer to perform the fork() before
> +	 * the process opens a wayland connection.
> +	 */
> +	if (pipe(pipes) == -1) {
> +		fprintf(stderr, "Can't create pipe for pacing.\n");
> +		exit(EXIT_FAILURE);
> +	}
>   
>   	pid = forkpty(&master, NULL, NULL, NULL);
>   	if (pid == 0) {
> +		int ret;
> +
> +		close(pipes[1]);
> +		do {
> +			char tmp;
> +			ret = read(pipes[0], &tmp, 1);
> +		} while (ret == -1 && errno == EINTR);

Shouldn’t the child close(pipes[0]); here?
With that fixed or answered:
Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>

Patches 1 and 2 reviewed and pushed:
5ef6bd7e..091c8017  master -> master

Thanks,


>   		setenv("TERM", option_term, 1);
>   		setenv("COLORTERM", option_term, 1);
>   		if (execl(path, path, NULL)) {
> @@ -3041,7 +3071,9 @@ terminal_run(struct terminal *terminal, const char *path)
>   		return -1;
>   	}
>   
> +	close(pipes[0]);
>   	terminal->master = master;
> +	terminal->pace_pipe = pipes[1];
>   	fcntl(master, F_SETFL, O_NONBLOCK);
>   	terminal->io_task.run = io_handler;
>   	display_watch_fd(terminal->display, terminal->master,
> 


-- 

Quentin “Sardem FF7” Glidic