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 <[email protected]>
Acked-by: Pekka Paalanen <[email protected]>
---
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
