[PATCH weston] terminal: Fix segmentation fault when processing DCH ANSI escape code

[Bryce Harrington]

On Sat, Jul 02, 2016 at 12:35:21AM +0000, aaron at correspondwith.me wrote:
> Fix a segfault where terminal parses a DCH escape code and the cursor is at the end of the line, causing terminal_shift_line to memmove memory it doesn't own.
> ---
>  clients/terminal.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/clients/terminal.c b/clients/terminal.c
> index 6257cb7..7a0da29 100644
> --- a/clients/terminal.c
> +++ b/clients/terminal.c
> @@ -724,13 +724,16 @@ terminal_shift_line(struct terminal *terminal, int d)
>  
>  	if (d < 0) {
>  		d = 0 - d;
> -		memmove(&row[terminal->column],
> -		        &row[terminal->column + d],
> -			(terminal->width - terminal->column - d) * sizeof(union utf8_char));
> -		memmove(&attr_row[terminal->column], &attr_row[terminal->column + d],
> -		        (terminal->width - terminal->column - d) * sizeof(struct attr));
> -		memset(&row[terminal->width - d], 0, d * sizeof(union utf8_char));
> -		attr_init(&attr_row[terminal->width - d], terminal->curr_attr, d);
> +
> +		if(terminal->width - terminal->column) {

style nit - space needed between the if and (

I might go with a != rather than a - since really all you care about is
whether the two parameters differ.  That would be keeping stylistically
consistent with the surrounding code in terminal.c which appears to
prefer boolean operations in the conditionals.

Although I notice we're checking d, that is already a calculation of
terminal->width and terminal->column, which makes me wonder if the
conditionals ought to be tweaked further rather than adding another if
statement.

> +			memmove(&row[terminal->column],
> +				&row[terminal->column + d],
> +				(terminal->width - terminal->column - d) * sizeof(union utf8_char));
> +			memmove(&attr_row[terminal->column], &attr_row[terminal->column + d],
> +				(terminal->width - terminal->column - d) * sizeof(struct attr));
> +			memset(&row[terminal->width - d], 0, d * sizeof(union utf8_char));
> +			attr_init(&attr_row[terminal->width - d], terminal->curr_attr, d);
> +		}

This does though leave the question of what the behavior should be if
the column number is equal to the width.  This just skips the lineshift,
right?  For DCH that's probably the right behavior, but can other codes
trigger this situation, and if so would they require different handling
here?

Thanks for reporting this; if the above refactoring doesn't sound like
work you want to do, that's fine - let me know and I'll look further
into it myself later on.

Bryce

>  	} else {
>  		memmove(&row[terminal->column + d], &row[terminal->column],
>  			(terminal->width - terminal->column - d) * sizeof(union utf8_char));
> -- 
> 2.9.0
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel