connection: add sanity check to avoid buffer overflow

[Derek Foreman]

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?

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?

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
>