Re: connection: add sanity check to avoid buffer overflow
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 <mailto:boram1288.p...@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 mailto:boram1288.p...@samsung.com>>
>>> Acked-by: Pekka Paalanen mailto:pekka.paala...@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 http://gmail.com>>
>> ___
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
<mailto:wayland-devel@lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
<mailto:wayland-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
<mailto:wayland-devel@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: connection: add sanity check to avoid buffer overflow
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
Acked-by: Pekka Paalanen
---
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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] connection: add sanity check to avoid buffer overflow
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
Acked-by: Pekka Paalanen
---
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);
+
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;
}
--
1.9.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] client: use the thread reader_count to fix deadlock.
This patch solves the deadlock issue of the below scenario.
1. A thread polls several fds including the wayland's fd.
2. This thread probably calls wl_display_prepare_read() before polling
fds.
3. This thread could be awake by other event source which isn't related
with wayland fd.
4. After wake up, this thread could call wl_display_dispatch or
wl_display_roundtrip for sync operation.
5. Then, when this thread got a done event. this thread will stuck in
deadlock because this thread increases +2 reader_count in the same
thread.
The read_event or cancel_read for the first prepare_read is not going to
happen because this thread sleeps by pthread_cond_wait() of read_event.
This problem can be solved by using the reader_count per thread.
The thread reader_count will be increased/decreased whenever prepare_read,
cancel_read and read_event are called.
However, the original display reader_count will be increased only once per
thread. And, when cancel_read and read_event are called, it will be
decreased for this thread to read event from fd and wake up other threads.
After that, if the thread reader_count is still more than 0, it will be
increased because it means this thread is still polling in somewhere.
Signed-off-by: Boram Park
---
src/wayland-client.c | 79 +++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 33033e7..3b80dfa 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -77,6 +77,10 @@ struct wl_event_queue {
struct wl_display *display;
};
+struct wl_thread_data {
+ int reader_count_in_thread;
+};
+
struct wl_display {
struct wl_proxy proxy;
struct wl_connection *connection;
@@ -107,6 +111,8 @@ struct wl_display {
int reader_count;
uint32_t read_serial;
pthread_cond_t reader_cond;
+
+ pthread_key_t thread_data_key;
};
/** \endcond */
@@ -785,6 +791,31 @@ wl_proxy_marshal_array(struct wl_proxy *proxy, uint32_t
opcode,
}
static void
+destroy_thread_data(void *data)
+{
+ struct wl_thread_data *thread_data = data;
+
+ free(thread_data);
+}
+
+static struct wl_thread_data*
+get_thread_data(struct wl_display *display)
+{
+ struct wl_thread_data *thread_data;
+
+ thread_data = pthread_getspecific(display->thread_data_key);
+ if (!thread_data) {
+ thread_data = zalloc(sizeof *thread_data);
+ if (!thread_data)
+ return NULL;
+ thread_data->reader_count_in_thread = 0;
+ pthread_setspecific(display->thread_data_key, thread_data);
+ }
+
+ return thread_data;
+}
+
+static void
display_handle_error(void *data,
struct wl_display *display, void *object,
uint32_t code, const char *message)
@@ -905,6 +936,7 @@ WL_EXPORT struct wl_display *
wl_display_connect_to_fd(int fd)
{
struct wl_display *display;
+ struct wl_thread_data *thread_data;
const char *debug;
debug = getenv("WAYLAND_DEBUG");
@@ -960,6 +992,13 @@ wl_display_connect_to_fd(int fd)
if (display->connection == NULL)
goto err_connection;
+ if (pthread_key_create(&display->thread_data_key, destroy_thread_data)
< 0)
+ goto err_connection;
+
+ thread_data = get_thread_data(display);
+ if (!thread_data)
+ goto err_connection;
+
return display;
err_connection:
@@ -1023,6 +1062,12 @@ wl_display_connect(const char *name)
WL_EXPORT void
wl_display_disconnect(struct wl_display *display)
{
+ struct wl_thread_data *thread_data;
+
+ thread_data = get_thread_data(display);
+ free(thread_data);
+ pthread_key_delete(display->thread_data_key);
+
wl_connection_destroy(display->connection);
wl_map_release(&display->objects);
wl_event_queue_release(&display->default_queue);
@@ -1297,9 +1342,14 @@ dispatch_event(struct wl_display *display, struct
wl_event_queue *queue)
static int
read_events(struct wl_display *display)
{
+ struct wl_thread_data *thread_data;
int total, rem, size;
uint32_t serial;
+ thread_data = get_thread_data(display);
+ assert(thread_data);
+
+ thread_data->reader_count_in_thread--;
display->reader_count--;
if (display->reader_count == 0) {
total = wl_connection_read(display->connection);
@@ -1309,6 +1359,9 @@ read_events(struct wl_display *display)
* the reader_count dropped to 0 */
display_wakeup_threads(display);
+ if (thread_data->reader_count_in_thread > 0)
+ display->reader_count++;
+
