connection: add sanity check to avoid buffer overflow

[Mike Blumenkrantz]

On one hand it may be dangerous for the scenario that you've described, but
on the other hand why are you (or anyone) needing to call internal,
non-exported libwayland functions?

??

On Thu, Oct 19, 2017 at 3:11 AM Boram Park <boram1288.park at samsung.com>
wrote:

>
>
> On 2017년 09월 28일 00:13, Derek Foreman wrote:
> > On 2017-09-26 10:46 AM, Sergi Granell wrote:
> >> On Thu, 2017-09-14 at 09:21 +0900, Boram Park wrote:
> >>> Before putting data into a buffer, we have to make sure that the data
> >>> size is
> >>> smaller than not only the buffer's full size but also the buffer's
> >>> empty
> >>> size.
> >>>
> >>> https://bugs.freedesktop.org/show_bug.cgi?id=102690
> >>>
> >>> Signed-off-by: Boram Park <boram1288.park at samsung.com>
> >>> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >>> ---
> >>>   src/connection.c | 9 ++++++---
> >>>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/src/connection.c b/src/connection.c
> >>> index 5c3d187..53b1621 100644
> >>> --- a/src/connection.c
> >>> +++ b/src/connection.c
> >>> @@ -63,14 +63,17 @@ struct wl_connection {
> >>>       int want_flush;
> >>>   };
> >>>   +static uint32_t wl_buffer_size(struct wl_buffer *b);
> >>> +
> >>
> >> I think it would be a better idea to move the wl_buffer_size definition
> >> at the top to avoid this forward declaration.
> >>
> >>>   static int
> >>>   wl_buffer_put(struct wl_buffer *b, const void *data, size_t count)
> >>>   {
> >>> -    uint32_t head, size;
> >>> +    uint32_t head, size, empty;
> >>>   -    if (count > sizeof(b->data)) {
> >>> +    empty = sizeof(b->data) - wl_buffer_size(b);
> >>> +    if (count > empty) {
> >>>           wl_log("Data too big for buffer (%d > %d).\n",
> >>> -               count, sizeof(b->data));
> >>> +               count, empty);
> >>>           errno = E2BIG;
> >>>           return -1;
> >>>       }
> >
> > I'm not sure I like this.  I've looked and all callers should already
> > have this check - are you actually getting here with this condition
> > somehow?
> looks it will never happen because all callers already check the data
> size before putting.
> > Also, the patch changes the meaning of E2BIG from the caller's
> > perspective (if we can even get here), doesn't it? Previously E2BIG
> > meant the packet could never fit, now it would mean that the packet
> > can't fit now.
> >
> > I think maybe just a comment mentioning that all the callers must
> > ensure the data will fit could be enough?
> However, it looks really dangerous for someone who don't know above rule
> that caller should check the buffer empty size before calling
> wl_buffer_put.
> comment might be helpful to understand the intention of developer who
> implemented this code.
> But the sanity check is much better to ensure safety, I think.
>
> >
> > I could even see an assert(), since this is a conditional that should
> > never fire.
> >
> > But I really don't like changing the meaning of the error code.
> >
> > Thanks,
> > Derek
> >
> >> Other than that,
> >>
> >> Reviewed-by: Sergi Granell <xerpi.g.12 at gmail.com>
> >> _______________________________________________
> >> wayland-devel mailing list
> >> wayland-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >>
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171019/1773b084/attachment.html>