[PATCH wayland] connection: Don't write past the end of the connection buffer

[Jasper St. Pierre]

On Thu, Apr 17, 2014 at 11:20 AM, Ander Conselvan de Oliveira <
conselvan2 at gmail.com> wrote:

> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
>
> If a message was too big to fit in the connection buffer, the code
> in wl_buffer_put would just write past the end of it.
>
> I haven't seen any real world use case that would trigger this bug, but
> it was possible to trigger it by sending a long enough string to the
> wl_data_source.offer request.
>

This will flat out kill the client, no? It seems like we should be able to
handle that more gracefully; does the client have any idea what the buffer
size is?


> https://bugs.freedesktop.org/show_bug.cgi?id=69267
> ---
>  src/connection.c        | 25 +++++++++++++++++--------
>  tests/connection-test.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index 40a2182..63b0592 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -61,11 +61,18 @@ struct wl_connection {
>         int want_flush;
>  };
>
> -static void
> +static int
>  wl_buffer_put(struct wl_buffer *b, const void *data, size_t count)
>  {
>         uint32_t head, size;
>
> +       if (count > sizeof(b->data)) {
> +               wl_log("Data too big for buffer (%d > %d).\n",
> +                      count, sizeof(b->data));
> +               errno = E2BIG;
> +               return -1;
> +       }
> +
>         head = MASK(b->head);
>         if (head + count <= sizeof b->data) {
>                 memcpy(b->data + head, data, count);
> @@ -76,6 +83,8 @@ wl_buffer_put(struct wl_buffer *b, const void *data,
> size_t count)
>         }
>
>         b->head += count;
> +
> +       return 0;
>  }
>
>  static void
> @@ -243,8 +252,8 @@ decode_cmsg(struct wl_buffer *buffer, struct msghdr
> *msg)
>                         size /= sizeof(int32_t);
>                         for (i = 0; i < size; i++)
>                                 close(((int*)CMSG_DATA(cmsg))[i]);
> -               } else {
> -                       wl_buffer_put(buffer, CMSG_DATA(cmsg), size);
> +               } else if (wl_buffer_put(buffer, CMSG_DATA(cmsg), size) <
> 0) {
> +                               return -1;
>                 }
>         }
>
> @@ -350,7 +359,9 @@ wl_connection_write(struct wl_connection *connection,
>                         return -1;
>         }
>
> -       wl_buffer_put(&connection->out, data, count);
> +       if (wl_buffer_put(&connection->out, data, count) < 0)
> +               return -1;
> +
>         connection->want_flush = 1;
>
>         return 0;
> @@ -367,7 +378,7 @@ wl_connection_queue(struct wl_connection *connection,
>                         return -1;
>         }
>
> -       wl_buffer_put(&connection->out, data, count);
> +       return wl_buffer_put(&connection->out, data, count);
>
>         return 0;
>

You forgot to remove the "return 0;" here.


>  }
> @@ -394,9 +405,7 @@ wl_connection_put_fd(struct wl_connection *connection,
> int32_t fd)
>                         return -1;
>         }
>
> -       wl_buffer_put(&connection->fds_out, &fd, sizeof fd);
> -
> -       return 0;
> +       return wl_buffer_put(&connection->fds_out, &fd, sizeof fd);
>  }
>
>  const char *
> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 52d235d..1213875 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -235,6 +235,27 @@ expected_fail_marshal(int expected_error, const char
> *format, ...)
>         assert(errno == expected_error);
>  }
>
> +static void
> +expected_fail_marshal_send(struct marshal_data *data, int expected_error,
> +                          const char *format, ...)
> +{
> +       struct wl_closure *closure;
> +       static const uint32_t opcode = 4444;
> +       static struct wl_object sender = { NULL, NULL, 1234 };
> +       struct wl_message message = { "test", format, NULL };
> +       va_list ap;
> +
> +       va_start(ap, format);
> +       closure = wl_closure_vmarshal(&sender, opcode, ap, &message);
> +       va_end(ap);
> +
> +       assert(closure);
> +       assert(wl_closure_send(closure, data->write_connection) < 0);
> +       assert(errno == expected_error);
> +
> +       wl_closure_destroy(closure);
> +}
> +
>  TEST(connection_marshal_nullables)
>  {
>         struct marshal_data data;
> @@ -490,6 +511,22 @@ TEST(connection_marshal_alot)
>         release_marshal_data(&data);
>  }
>
> +TEST(connection_marshal_too_big)
> +{
> +       struct marshal_data data;
> +       char *big_string = malloc(5000);
> +
> +       memset(big_string, ' ', 4999);
> +       big_string[4999] = '\0';
> +
> +       setup_marshal_data(&data);
> +
> +       expected_fail_marshal_send(&data, E2BIG, "s", big_string);
> +
> +       release_marshal_data(&data);
> +       free(big_string);
> +}
> +
>  static void
>  marshal_helper(const char *format, void *handler, ...)
>  {
> --
> 1.8.3.2
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>



-- 
  Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140417/8c20c7da/attachment-0001.html>