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

[Daniel Stone]

Hi,

On 7 June 2016 at 03:29, Bryce Harrington  wrote:
> On Sun, Jun 05, 2016 at 07:01:11PM +0200, Benoit Gschwind wrote:
>> sprintf can overflow the fixed length title which is char[32]. This
>> patch change title to dynamically allocated char array using asprintf or
>> strdup. If one of them fail we leave returning NULL to indicate the
>> failure.
>>
>> Signed-of-by: Benoit Gschwind
>
> Yeah I had noticed this myself earlier and wondered if it should be
> dynamically allocated.  This looks like it would be a possible candidate
> for the stable branch if we do a .1.
>
> Reviewed-by: Bryce Harrington 

Pushed with reviews, thanks!
   0f1cac5..5c2f20e  upstream -> master

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

On Sun, Jun 05, 2016 at 07:01:11PM +0200, Benoit Gschwind wrote:
> sprintf can overflow the fixed length title which is char[32]. This
> patch change title to dynamically allocated char array using asprintf or
> strdup. If one of them fail we leave returning NULL to indicate the
> failure.
> 
> Signed-of-by: Benoit Gschwind

Yeah I had noticed this myself earlier and wondered if it should be
dynamically allocated.  This looks like it would be a possible candidate
for the stable branch if we do a .1.

Reviewed-by: Bryce Harrington 

> ---
> v3:
>  - fix xcb_destroy_window arguments
> 
> v2:
>  - fix spacing
>  - properly cleanup everything on failure.
> 
>  src/compositor-x11.c | 28 +++-
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 629b5f3..8727934 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;
>   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,24 @@ 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) {
> + if (asprintf(&title, "%s - %s", name, configured_name) < 0)
> + title = NULL;
> + } else {
> + title = strdup(name);
> + }
> +
> + 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 {
> + xcb_destroy_window(b->conn, output->window);
> + free(output);
> + return NULL;
> + }
> +
>   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
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Yong Bakos]

On Jun 5, 2016, at 12:01 PM, Benoit Gschwind  wrote:
> 
> sprintf can overflow the fixed length title which is char[32]. This
> patch change title to dynamically allocated char array using asprintf or
> strdup. If one of them fail we leave returning NULL to indicate the
> failure.
> 
> Signed-of-by: Benoit Gschwind

Reviewed-by: Yong Bakos
Tested-by: Yong Bakos

Cheers,
yong


> ---
> v3:
> - fix xcb_destroy_window arguments
> 
> v2:
> - fix spacing
> - properly cleanup everything on failure.
> 
> src/compositor-x11.c | 28 +++-
> 1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 629b5f3..8727934 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;
>   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,24 @@ 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) {
> + if (asprintf(&title, "%s - %s", name, configured_name) < 0)
> + title = NULL;
> + } else {
> + title = strdup(name);
> + }
> +
> + 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 {
> + xcb_destroy_window(b->conn, output->window);
> + free(output);
> + return NULL;
> + }
> +
>   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

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel