* [PATCH v3 2/5] unit: Add race condition test to test-main
2016-03-17 18:15 [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
@ 2016-03-17 18:15 ` Mat Martineau
2016-03-17 18:17 ` Mat Martineau
2016-03-17 18:19 ` [PATCH v4 " Mat Martineau
2016-03-17 18:15 ` [PATCH v3 3/5] main: Safely free watch_data structures Mat Martineau
` (3 subsequent siblings)
4 siblings, 2 replies; 10+ messages in thread
From: Mat Martineau @ 2016-03-17 18:15 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]
This test excercises the case where multiple events are returned from
the same epoll_wait call and an early event deletes a watch that is
waiting to be processed in the same event loop iteration.
---
unit/test-main.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/unit/test-main.c b/unit/test-main.c
index b61e337..469d186 100644
--- a/unit/test-main.c
+++ b/unit/test-main.c
@@ -25,6 +25,7 @@
#endif
#include <ell/ell.h>
+#include <unistd.h>
static void signal_handler(struct l_signal *signal, uint32_t signo,
void *user_data)
@@ -38,7 +39,7 @@ static void signal_handler(struct l_signal *signal, uint32_t signo,
}
}
-static void timeout_handler(struct l_timeout *timeout, void *user_data)
+static void timeout_quit_handler(struct l_timeout *timeout, void *user_data)
{
l_main_quit();
}
@@ -58,9 +59,27 @@ static void oneshot_handler(void *user_data)
l_info("One-shot");
}
+static void race_delay_handler(struct l_timeout *timeout, void *user_data)
+{
+ l_info("Delay");
+ usleep(250 * 1000);
+}
+
+static void race_handler(struct l_timeout *timeout, void *user_data)
+{
+ struct l_timeout **other_racer = user_data;
+
+ l_info("Remove pending event");
+ l_timeout_remove(*other_racer);
+ *other_racer = NULL;
+}
+
int main(int argc, char *argv[])
{
- struct l_timeout *timeout;
+ struct l_timeout *timeout_quit;
+ struct l_timeout *race_delay;
+ struct l_timeout *race1;
+ struct l_timeout *race2;
struct l_signal *signal;
struct l_idle *idle;
sigset_t mask;
@@ -71,7 +90,13 @@ int main(int argc, char *argv[])
signal = l_signal_create(&mask, signal_handler, NULL, NULL);
- timeout = l_timeout_create(3, timeout_handler, NULL, NULL);
+ timeout_quit = l_timeout_create(3, timeout_quit_handler, NULL, NULL);
+
+ race_delay = l_timeout_create(1, race_delay_handler, NULL, NULL);
+ race1 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
+ &race2, NULL);
+ race2 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
+ &race1, NULL);
idle = l_idle_create(idle_handler, NULL, NULL);
@@ -85,7 +110,11 @@ int main(int argc, char *argv[])
l_main_run();
- l_timeout_remove(timeout);
+ l_timeout_remove(race_delay);
+ l_timeout_remove(race1);
+ l_timeout_remove(race2);
+
+ l_timeout_remove(timeout_quit);
l_signal_remove(signal);
--
2.7.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 2/5] unit: Add race condition test to test-main
2016-03-17 18:15 ` [PATCH v3 2/5] unit: Add race condition test to test-main Mat Martineau
@ 2016-03-17 18:17 ` Mat Martineau
2016-03-17 18:19 ` [PATCH v4 " Mat Martineau
1 sibling, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2016-03-17 18:17 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 323 bytes --]
On Thu, 17 Mar 2016, Mat Martineau wrote:
> This test excercises the case where multiple events are returned from
Oops, should be "exercises"
> the same epoll_wait call and an early event deletes a watch that is
> waiting to be processed in the same event loop iteration.
--
Mat Martineau
Intel OTC
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/5] unit: Add race condition test to test-main
2016-03-17 18:15 ` [PATCH v3 2/5] unit: Add race condition test to test-main Mat Martineau
2016-03-17 18:17 ` Mat Martineau
@ 2016-03-17 18:19 ` Mat Martineau
2016-03-17 18:35 ` Denis Kenzior
1 sibling, 1 reply; 10+ messages in thread
From: Mat Martineau @ 2016-03-17 18:19 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]
This test exercises the case where multiple events are returned from
the same epoll_wait call and an early event deletes a watch that is
waiting to be processed in the same event loop iteration.
---
unit/test-main.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/unit/test-main.c b/unit/test-main.c
index b61e337..469d186 100644
--- a/unit/test-main.c
+++ b/unit/test-main.c
@@ -25,6 +25,7 @@
#endif
#include <ell/ell.h>
+#include <unistd.h>
static void signal_handler(struct l_signal *signal, uint32_t signo,
void *user_data)
@@ -38,7 +39,7 @@ static void signal_handler(struct l_signal *signal, uint32_t signo,
}
}
-static void timeout_handler(struct l_timeout *timeout, void *user_data)
+static void timeout_quit_handler(struct l_timeout *timeout, void *user_data)
{
l_main_quit();
}
@@ -58,9 +59,27 @@ static void oneshot_handler(void *user_data)
l_info("One-shot");
}
+static void race_delay_handler(struct l_timeout *timeout, void *user_data)
+{
+ l_info("Delay");
+ usleep(250 * 1000);
+}
+
+static void race_handler(struct l_timeout *timeout, void *user_data)
+{
+ struct l_timeout **other_racer = user_data;
+
+ l_info("Remove pending event");
+ l_timeout_remove(*other_racer);
+ *other_racer = NULL;
+}
+
int main(int argc, char *argv[])
{
- struct l_timeout *timeout;
+ struct l_timeout *timeout_quit;
+ struct l_timeout *race_delay;
+ struct l_timeout *race1;
+ struct l_timeout *race2;
struct l_signal *signal;
struct l_idle *idle;
sigset_t mask;
@@ -71,7 +90,13 @@ int main(int argc, char *argv[])
signal = l_signal_create(&mask, signal_handler, NULL, NULL);
- timeout = l_timeout_create(3, timeout_handler, NULL, NULL);
+ timeout_quit = l_timeout_create(3, timeout_quit_handler, NULL, NULL);
+
+ race_delay = l_timeout_create(1, race_delay_handler, NULL, NULL);
+ race1 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
+ &race2, NULL);
+ race2 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
+ &race1, NULL);
idle = l_idle_create(idle_handler, NULL, NULL);
@@ -85,7 +110,11 @@ int main(int argc, char *argv[])
l_main_run();
- l_timeout_remove(timeout);
+ l_timeout_remove(race_delay);
+ l_timeout_remove(race1);
+ l_timeout_remove(race2);
+
+ l_timeout_remove(timeout_quit);
l_signal_remove(signal);
--
2.7.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/5] main: Safely free watch_data structures
2016-03-17 18:15 [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
2016-03-17 18:15 ` [PATCH v3 2/5] unit: Add race condition test to test-main Mat Martineau
@ 2016-03-17 18:15 ` Mat Martineau
2016-03-17 18:35 ` Denis Kenzior
2016-03-17 18:15 ` [PATCH v3 4/5] main: Remove unnecessary error check Mat Martineau
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Mat Martineau @ 2016-03-17 18:15 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]
The race condition test in test-main exposed a case where the events
array returned by epoll_wait could have a stale watch_data
pointer. This triggered a use-after-free error that was reported by
the address sanitizer (./configure --enable-asan).
When the event loop is running, watch_data structures with events
being dispatched are flagged. If watch_remove is called on a flagged
structure, the l_free() is deferred until after all events are
dispatched and the callback for that event is skipped.
---
ell/main.c | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/ell/main.c b/ell/main.c
index f8fa4ac..6143a42 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -49,6 +49,9 @@
#define IDLE_FLAG_DISPATCHING 1
#define IDLE_FLAG_DESTROYED 2
+#define WATCH_FLAG_DISPATCHING 1
+#define WATCH_FLAG_DESTROYED 2
+
static int epoll_fd;
static bool epoll_running;
static bool epoll_terminate;
@@ -60,6 +63,7 @@ static struct l_queue *idle_list;
struct watch_data {
int fd;
uint32_t events;
+ uint32_t flags;
watch_event_cb_t callback;
watch_destroy_cb_t destroy;
void *user_data;
@@ -139,6 +143,7 @@ int watch_add(int fd, uint32_t events, watch_event_cb_t callback,
data->fd = fd;
data->events = events;
+ data->flags = 0;
data->callback = callback;
data->destroy = destroy;
data->user_data = user_data;
@@ -212,7 +217,10 @@ int watch_remove(int fd)
if (data->destroy)
data->destroy(data->user_data);
- l_free(data);
+ if (data->flags & WATCH_FLAG_DISPATCHING)
+ data->flags |= WATCH_FLAG_DESTROYED;
+ else
+ l_free(data);
return err;
}
@@ -334,6 +342,7 @@ LIB_EXPORT int l_main_run(void)
for (;;) {
struct epoll_event events[MAX_EPOLL_EVENTS];
+ struct watch_data *data;
int n, nfds;
int timeout;
@@ -344,12 +353,30 @@ LIB_EXPORT int l_main_run(void)
nfds = epoll_wait(epoll_fd, events, MAX_EPOLL_EVENTS, timeout);
for (n = 0; n < nfds; n++) {
- struct watch_data *data = events[n].data.ptr;
+ data = events[n].data.ptr;
+
+ data->flags |= WATCH_FLAG_DISPATCHING;
+ }
+
+ for (n = 0; n < nfds; n++) {
+ data = events[n].data.ptr;
+
+ if (data->flags & WATCH_FLAG_DESTROYED)
+ continue;
data->callback(data->fd, events[n].events,
data->user_data);
}
+ for (n = 0; n < nfds; n++) {
+ data = events[n].data.ptr;
+
+ if (data->flags & WATCH_FLAG_DESTROYED)
+ l_free(data);
+ else
+ data->flags = 0;
+ }
+
l_queue_foreach(idle_list, idle_dispatch, NULL);
l_queue_foreach_remove(idle_list, idle_prune, NULL);
}
--
2.7.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 3/5] main: Safely free watch_data structures
2016-03-17 18:15 ` [PATCH v3 3/5] main: Safely free watch_data structures Mat Martineau
@ 2016-03-17 18:35 ` Denis Kenzior
0 siblings, 0 replies; 10+ messages in thread
From: Denis Kenzior @ 2016-03-17 18:35 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
Hi Mat,
On 03/17/2016 01:15 PM, Mat Martineau wrote:
> The race condition test in test-main exposed a case where the events
> array returned by epoll_wait could have a stale watch_data
> pointer. This triggered a use-after-free error that was reported by
> the address sanitizer (./configure --enable-asan).
>
> When the event loop is running, watch_data structures with events
> being dispatched are flagged. If watch_remove is called on a flagged
> structure, the l_free() is deferred until after all events are
> dispatched and the callback for that event is skipped.
> ---
> ell/main.c | 31 +++++++++++++++++++++++++++++--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
Patches 3-5 applied. Thanks.
Regards,
-Denis
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 4/5] main: Remove unnecessary error check
2016-03-17 18:15 [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
2016-03-17 18:15 ` [PATCH v3 2/5] unit: Add race condition test to test-main Mat Martineau
2016-03-17 18:15 ` [PATCH v3 3/5] main: Safely free watch_data structures Mat Martineau
@ 2016-03-17 18:15 ` Mat Martineau
2016-03-17 18:15 ` [PATCH v3 5/5] unit: Add test-main check for self-removal Mat Martineau
2016-03-25 21:51 ` [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
4 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2016-03-17 18:15 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 714 bytes --]
l_queue_new() cannot fail, so there's no need to check for NULL or do
other cleanup.
---
ell/main.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/ell/main.c b/ell/main.c
index 6143a42..46e929a 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -100,8 +100,6 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
goto close_epoll;
idle_list = l_queue_new();
- if (!idle_list)
- goto free_watch_list;
idle_id = 0;
@@ -112,10 +110,6 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
return true;
-free_watch_list:
- free(watch_list);
- watch_list = NULL;
-
close_epoll:
close(epoll_fd);
epoll_fd = 0;
--
2.7.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 5/5] unit: Add test-main check for self-removal
2016-03-17 18:15 [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
` (2 preceding siblings ...)
2016-03-17 18:15 ` [PATCH v3 4/5] main: Remove unnecessary error check Mat Martineau
@ 2016-03-17 18:15 ` Mat Martineau
2016-03-25 21:51 ` [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
4 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2016-03-17 18:15 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]
Call l_timeout_remove() within a handler for the same timeout.
---
unit/test-main.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/unit/test-main.c b/unit/test-main.c
index 469d186..ee99a4b 100644
--- a/unit/test-main.c
+++ b/unit/test-main.c
@@ -74,12 +74,19 @@ static void race_handler(struct l_timeout *timeout, void *user_data)
*other_racer = NULL;
}
+static void remove_handler(struct l_timeout *timeout, void *user_data)
+{
+ l_timeout_remove(timeout);
+ l_info("Timer removed itself");
+}
+
int main(int argc, char *argv[])
{
struct l_timeout *timeout_quit;
struct l_timeout *race_delay;
struct l_timeout *race1;
struct l_timeout *race2;
+ struct l_timeout *remove_self;
struct l_signal *signal;
struct l_idle *idle;
sigset_t mask;
@@ -98,6 +105,8 @@ int main(int argc, char *argv[])
race2 = l_timeout_create_with_nanoseconds(1, 100000000, race_handler,
&race1, NULL);
+ remove_self = l_timeout_create(2, remove_handler, &remove_self, NULL);
+
idle = l_idle_create(idle_handler, NULL, NULL);
l_log_set_stderr();
--
2.7.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 1/5] build: Add sanitizer options
2016-03-17 18:15 [PATCH v3 1/5] build: Add sanitizer options Mat Martineau
` (3 preceding siblings ...)
2016-03-17 18:15 ` [PATCH v3 5/5] unit: Add test-main check for self-removal Mat Martineau
@ 2016-03-25 21:51 ` Mat Martineau
4 siblings, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2016-03-25 21:51 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 3938 bytes --]
Marcel - any issues with this patch?
On Thu, 17 Mar 2016, Mat Martineau wrote:
> Build using Address Sanitizer (asan), Leak Sanitizer (lsan), or
> Undefined Behavior Sanitizer (ubsan) by using one of these options for
> the configure script:
>
> --enable-asan
> --enable-lsan
> --enable-ubsan
>
> For each of these to work, the compiler must support the requested
> sanitizer and the requisite libraries must be installed (libasan,
> liblsan, libubsan).
> ---
> acinclude.m4 | 36 ++++++++++++++++++++++++++++++++++++
> configure.ac | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 81 insertions(+)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 0ba4287..8aab4ee 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -10,6 +10,42 @@ AC_DEFUN([AC_PROG_CC_PIE], [
> ])
> ])
>
> +AC_DEFUN([AC_PROG_CC_ASAN], [
> + AC_CACHE_CHECK([whether ${CC-cc} accepts -fsanitize=address], ac_cv_prog_cc_asan, [
> + echo 'void f(){}' > conftest.c
> + if test -z "`${CC-cc} -fsanitize=address -c conftest.c 2>&1`"; then
> + ac_cv_prog_cc_asan=yes
> + else
> + ac_cv_prog_cc_asan=no
> + fi
> + rm -rf conftest*
> + ])
> +])
> +
> +AC_DEFUN([AC_PROG_CC_LSAN], [
> + AC_CACHE_CHECK([whether ${CC-cc} accepts -fsanitize=leak], ac_cv_prog_cc_lsan, [
> + echo 'void f(){}' > conftest.c
> + if test -z "`${CC-cc} -fsanitize=leak -c conftest.c 2>&1`"; then
> + ac_cv_prog_cc_lsan=yes
> + else
> + ac_cv_prog_cc_lsan=no
> + fi
> + rm -rf conftest*
> + ])
> +])
> +
> +AC_DEFUN([AC_PROG_CC_UBSAN], [
> + AC_CACHE_CHECK([whether ${CC-cc} accepts -fsanitize=undefined], ac_cv_prog_cc_ubsan, [
> + echo 'void f(){}' > conftest.c
> + if test -z "`${CC-cc} -fsanitize=undefined -c conftest.c 2>&1`"; then
> + ac_cv_prog_cc_ubsan=yes
> + else
> + ac_cv_prog_cc_ubsan=no
> + fi
> + rm -rf conftest*
> + ])
> +])
> +
> AC_DEFUN([COMPILER_FLAGS], [
> if (test "${CFLAGS}" = ""); then
> CFLAGS="-Wall -O2 -fsigned-char"
> diff --git a/configure.ac b/configure.ac
> index 39755fe..e4ca5b2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -20,6 +20,9 @@ AC_LANG_C
>
> AC_PROG_CC
> AC_PROG_CC_PIE
> +AC_PROG_CC_ASAN
> +AC_PROG_CC_LSAN
> +AC_PROG_CC_UBSAN
> AC_PROG_INSTALL
>
> AC_C_CHAR_UNSIGNED
> @@ -54,6 +57,48 @@ AC_ARG_ENABLE(pie, AC_HELP_STRING([--enable-pie],
> fi
> ])
>
> +save_LIBS=$LIBS
> +AC_CHECK_LIB(asan, __sanitizer_cov_init)
> +LIBS=$save_LIBS
> +
> +AC_ARG_ENABLE(asan, AC_HELP_STRING([--enable-asan],
> + [enable linking with address sanitizer]), [
> + if (test "${enableval}" = "yes" &&
> + test "${ac_cv_lib_asan___sanitizer_cov_init}" = "yes" &&
> + test "${ac_cv_prog_cc_asan}" = "yes"); then
> + CFLAGS="$CFLAGS -fsanitize=address";
> + LDFLAGS="$LDFLAGS -fsanitize=address"
> + fi
> +])
> +
> +save_LIBS=$LIBS
> +AC_CHECK_LIB(lsan, __sanitizer_cov_init)
> +LIBS=$save_LIBS
> +
> +AC_ARG_ENABLE(lsan, AC_HELP_STRING([--enable-lsan],
> + [enable linking with leak sanitizer]), [
> + if (test "${enableval}" = "yes" &&
> + test "${ac_cv_lib_lsan___sanitizer_cov_init}" = "yes" &&
> + test "${ac_cv_prog_cc_lsan}" = "yes"); then
> + CFLAGS="$CFLAGS -fsanitize=leak";
> + LDFLAGS="$LDFLAGS -fsanitize=leak"
> + fi
> +])
> +
> +save_LIBS=$LIBS
> +AC_CHECK_LIB(ubsan, __sanitizer_cov_init)
> +LIBS=$save_LIBS
> +
> +AC_ARG_ENABLE(ubsan, AC_HELP_STRING([--enable-ubsan],
> + [enable linking with undefined behavior sanitizer]), [
> + if (test "${enableval}" = "yes" &&
> + test "${ac_cv_lib_ubsan___sanitizer_cov_init}" = "yes" &&
> + test "${ac_cv_prog_cc_ubsan}" = "yes"); then
> + CFLAGS="$CFLAGS -fsanitize=undefined";
> + LDFLAGS="$LDFLAGS -fsanitize=undefined"
> + fi
> +])
> +
> AC_CHECK_FUNC(signalfd, dummy=yes,
> AC_MSG_ERROR(signalfd support is required))
>
> --
> 2.7.3
>
>
--
Mat Martineau
Intel OTC
^ permalink raw reply [flat|nested] 10+ messages in thread