[PATCH v1] compositor-x11: fix title overflow in x11_backend_create_output

[Benoit Gschwind]

Hello Yong,

Thanks for the review.

On 05/06/2016 17:47, Yong Bakos wrote:
> Hi Benoit,
> Other than the comments inline below, this patch is
> Reviewed-by: Yong Bakos <ybakos at humanoriented.com>
> Tested-by: Yong Bakos <ybakos at humanoriented.com>
> 
> On Jun 4, 2016, at 9:08 AM, Benoit Gschwind <gschwind at gnu-log.net> wrote:
>>
>> ---
> 
> I'd love to see a short message for the patch here explaining the
> rationale and behavior change, now that it's possible to return NULL,
> causing the compositor to terminate.
> 
> 

I can add more line here, but there is no behavior change, NULL pointer
is returned on malloc failure few line above, thus returning NULL for
the same issue here seems fine. That said, I made a mistake, because I
do not free the output before leaving.

Note also that I do not expect the code to be able to go far further if
we cannot allocate the string buffer here, that why I choose to leave
immediately. But I may be wrong.

>> src/compositor-x11.c | 29 ++++++++++++++++++++---------
>> 1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
>> index 629b5f3..a518820 100644
>> --- a/src/compositor-x11.c
>> +++ b/src/compositor-x11.c
>> @@ -782,7 +782,7 @@ x11_backend_create_output(struct x11_backend *b, int x, int y,
>> {
>> 	static const char name[] = "Weston Compositor";
>> 	static const char class[] = "weston-1\0Weston Compositor";
>> -	char title[32];
>> +	char * title = NULL;
> 
> char *title = NULL;
> 
> 
>> 	struct x11_output *output;
>> 	xcb_screen_t *screen;
>> 	struct wm_normal_hints normal_hints;
>> @@ -800,11 +800,6 @@ x11_backend_create_output(struct x11_backend *b, int x, int y,
>> 	output_width = width * scale;
>> 	output_height = height * scale;
>>
>> -	if (configured_name)
>> -		sprintf(title, "%s - %s", name, configured_name);
>> -	else
>> -		strcpy(title, name);
>> -
>> 	if (!no_input)
>> 		values[0] |=
>> 			XCB_EVENT_MASK_KEY_PRESS |
>> @@ -871,9 +866,25 @@ x11_backend_create_output(struct x11_backend *b, int x, int y,
>> 	}
>>
>> 	/* Set window name.  Don't bother with non-EWMH WMs. */
>> -	xcb_change_property(b->conn, XCB_PROP_MODE_REPLACE, output->window,
>> -			    b->atom.net_wm_name, b->atom.utf8_string, 8,
>> -			    strlen(title), title);
>> +	if (configured_name) {
>> +		ret = asprintf(&title, "%s - %s", name, configured_name);
>> +		 /* following asprintf manual, title is undefined on failure,
>> +		  * thus force NULL */
>> +		if (ret < 0)
>> +			title = NULL;
>> +	} else {
>> +		title = strdup(name);
>> +	}
>> +
>> +	if(title) {
> 
> if (title) {
> 
>> +		xcb_change_property(b->conn, XCB_PROP_MODE_REPLACE, output->window,
>> +				    b->atom.net_wm_name, b->atom.utf8_string, 8,
>> +				    strlen(title), title);
>> +		free(title);
>> +	} else {
>> +		return NULL;
> 
> Should we return NULL or just pass `name` to xcb_change_property?
> 
> 
>> +	}
>> +
>> 	xcb_change_property(b->conn, XCB_PROP_MODE_REPLACE, output->window,
>> 			    b->atom.wm_class, b->atom.string, 8,
>> 			    sizeof class, class);
>> -- 
>> 2.7.3
> 
> yong
> 
> 

Thanks, I will update my patch.

Best regards.