connection: add sanity check to avoid buffer overflow

[Boram Park]

wl_buffer_put is absolutely an internal static function. I don't plan to 
call or expose this function externally.
Before knowing the intent of the developer, it just seems to require the 
sanity checking code.
If the sanity check of the data size is the role of the caller, the code 
that checks whether the data size is larger than the total buffer size 
doesn't need to be in the wl_buffer_put either.

I hope that any code that may be misleading is clearly modified.

On 2017년 10월 19일 23:29, Mike Blumenkrantz wrote:
> 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 
> <mailto: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
>     <mailto:boram1288.park at samsung.com>>
>     >>> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk
>     <mailto: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
>     <http://gmail.com>>
>     >> _______________________________________________
>     >> wayland-devel mailing list
>     >> wayland-devel at lists.freedesktop.org
>     <mailto:wayland-devel at lists.freedesktop.org>
>     >> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>     >>
>     >
>     > _______________________________________________
>     > wayland-devel mailing list
>     > wayland-devel at lists.freedesktop.org
>     <mailto:wayland-devel at lists.freedesktop.org>
>     > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>     _______________________________________________
>     wayland-devel mailing list
>     wayland-devel at lists.freedesktop.org
>     <mailto: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/20171020/ccf009b2/attachment-0001.html>