[PATCH v2 1/3] build: Add sanitizer options

[PATCH v2 1/3] build: Add sanitizer options

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3585 bytes --]

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

[PATCH v2 2/3] unit: Add race condition test to test-main

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2425 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 | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/unit/test-main.c b/unit/test-main.c
index b61e337..26df8ad 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,25 @@ static void oneshot_handler(void *user_data)
 	l_info("One-shot");
 }
 
+static void race_delay_handler(struct l_timeout *timeout, void *user_data)
+{
+	usleep(250 * 1000);
+}
+
+static void race_handler(struct l_timeout *timeout, void *user_data)
+{
+	struct l_timeout **other_racer = user_data;
+
+	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 +88,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 +108,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

[PATCH v2 3/3] main: Safely free watch_data structures

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2713 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_remove now queues the watch_data
structure to be freed later. The watch_data callback is modified so
that a safe function is executed if the event loop attempts a callback
on a pending removed watch. Any queued watch_data structures are freed
at the end of each event loop iteration.
---
 ell/main.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/ell/main.c b/ell/main.c
index f8fa4ac..135a6da 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -56,6 +56,7 @@ static int idle_id;
 static int exit_status = EXIT_FAILURE;
 
 static struct l_queue *idle_list;
+static struct l_queue *watch_free_list;
 
 struct watch_data {
 	int fd;
@@ -101,6 +102,10 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
 
 	idle_id = 0;
 
+	watch_free_list = l_queue_new();
+	if (!watch_free_list)
+		goto free_idle_list;
+
 	watch_entries = DEFAULT_WATCH_ENTRIES;
 
 	for (i = 0; i < watch_entries; i++)
@@ -108,6 +113,10 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
 
 	return true;
 
+free_idle_list:
+	l_queue_destroy(idle_list, NULL);
+	idle_list = NULL;
+
 free_watch_list:
 	free(watch_list);
 	watch_list = NULL;
@@ -190,6 +199,11 @@ int watch_modify(int fd, uint32_t events, bool force)
 	return 0;
 }
 
+static void watch_nop_callback(int fd, uint32_t events, void *user_data)
+{
+	return;
+}
+
 int watch_remove(int fd)
 {
 	struct watch_data *data;
@@ -212,7 +226,15 @@ int watch_remove(int fd)
 	if (data->destroy)
 		data->destroy(data->user_data);
 
-	l_free(data);
+	if (epoll_running) {
+		/* The epoll events array may point to the same memory as
+		 * 'data', so let the event loop free it later
+		 */
+		data->callback = watch_nop_callback;
+		l_queue_push_tail(watch_free_list, data);
+	} else {
+		l_free(data);
+	}
 
 	return err;
 }
@@ -352,6 +374,7 @@ LIB_EXPORT int l_main_run(void)
 
 		l_queue_foreach(idle_list, idle_dispatch, NULL);
 		l_queue_foreach_remove(idle_list, idle_prune, NULL);
+		l_queue_clear(watch_free_list, l_free);
 	}
 
 	for (i = 0; i < watch_entries; i++) {
@@ -378,6 +401,9 @@ LIB_EXPORT int l_main_run(void)
 	l_queue_destroy(idle_list, idle_destroy);
 	idle_list = NULL;
 
+	l_queue_destroy(watch_free_list, l_free);
+	watch_free_list = NULL;
+
 	epoll_running = false;
 
 	close(epoll_fd);
-- 
2.7.3

Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

Hi Mat,

On 03/16/2016 03:01 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_remove now queues the watch_data
> structure to be freed later. The watch_data callback is modified so
> that a safe function is executed if the event loop attempts a callback
> on a pending removed watch. Any queued watch_data structures are freed
> at the end of each event loop iteration.
> ---
>   ell/main.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/ell/main.c b/ell/main.c
> index f8fa4ac..135a6da 100644
> --- a/ell/main.c
> +++ b/ell/main.c
> @@ -56,6 +56,7 @@ static int idle_id;
>   static int exit_status = EXIT_FAILURE;
>
>   static struct l_queue *idle_list;
> +static struct l_queue *watch_free_list;
>
>   struct watch_data {
>   	int fd;
> @@ -101,6 +102,10 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
>
>   	idle_id = 0;
>
> +	watch_free_list = l_queue_new();

l_queue_new cannot fail.

> +	if (!watch_free_list)
> +		goto free_idle_list;
> +
>   	watch_entries = DEFAULT_WATCH_ENTRIES;
>
>   	for (i = 0; i < watch_entries; i++)
> @@ -108,6 +113,10 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
>
>   	return true;
>
> +free_idle_list:
> +	l_queue_destroy(idle_list, NULL);
> +	idle_list = NULL;
> +
>   free_watch_list:
>   	free(watch_list);
>   	watch_list = NULL;
> @@ -190,6 +199,11 @@ int watch_modify(int fd, uint32_t events, bool force)
>   	return 0;
>   }
>
> +static void watch_nop_callback(int fd, uint32_t events, void *user_data)
> +{
> +	return;
> +}
> +
>   int watch_remove(int fd)
>   {
>   	struct watch_data *data;
> @@ -212,7 +226,15 @@ int watch_remove(int fd)
>   	if (data->destroy)
>   		data->destroy(data->user_data);
>
> -	l_free(data);
> +	if (epoll_running) {
> +		/* The epoll events array may point to the same memory as
> +		 * 'data', so let the event loop free it later
> +		 */
> +		data->callback = watch_nop_callback;
> +		l_queue_push_tail(watch_free_list, data);

Why don't we do this exactly how the idle events are handled.  E.g. set 
a DESTROYED flag and prune the event list after the epoll processing is 
complete.

> +	} else {
> +		l_free(data);
> +	}
>
>   	return err;
>   }
> @@ -352,6 +374,7 @@ LIB_EXPORT int l_main_run(void)
>
>   		l_queue_foreach(idle_list, idle_dispatch, NULL);
>   		l_queue_foreach_remove(idle_list, idle_prune, NULL);
> +		l_queue_clear(watch_free_list, l_free);
>   	}
>
>   	for (i = 0; i < watch_entries; i++) {
> @@ -378,6 +401,9 @@ LIB_EXPORT int l_main_run(void)
>   	l_queue_destroy(idle_list, idle_destroy);
>   	idle_list = NULL;
>
> +	l_queue_destroy(watch_free_list, l_free);
> +	watch_free_list = NULL;
> +
>   	epoll_running = false;
>
>   	close(epoll_fd);
>

Regards,
-Denis

Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 5400 bytes --]


On Wed, 16 Mar 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 03/16/2016 03:01 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_remove now queues the watch_data
>> structure to be freed later. The watch_data callback is modified so
>> that a safe function is executed if the event loop attempts a callback
>> on a pending removed watch. Any queued watch_data structures are freed
>> at the end of each event loop iteration.
>> ---
>>   ell/main.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>> 
>> diff --git a/ell/main.c b/ell/main.c
>> index f8fa4ac..135a6da 100644
>> --- a/ell/main.c
>> +++ b/ell/main.c
>> @@ -56,6 +56,7 @@ static int idle_id;
>>   static int exit_status = EXIT_FAILURE;
>>
>>   static struct l_queue *idle_list;
>> +static struct l_queue *watch_free_list;
>>
>>   struct watch_data {
>>   	int fd;
>> @@ -101,6 +102,10 @@ static inline bool __attribute__ ((always_inline)) 
>> create_epoll(void)
>>
>>   	idle_id = 0;
>> 
>> +	watch_free_list = l_queue_new();
>
> l_queue_new cannot fail.

Ok. I will remove the similar check for idle_list just above this (before 
the patch context) in a separate patch.

>
>> +	if (!watch_free_list)
>> +		goto free_idle_list;
>> +
>>   	watch_entries = DEFAULT_WATCH_ENTRIES;
>>
>>   	for (i = 0; i < watch_entries; i++)
>> @@ -108,6 +113,10 @@ static inline bool __attribute__ ((always_inline)) 
>> create_epoll(void)
>>
>>   	return true;
>> 
>> +free_idle_list:
>> +	l_queue_destroy(idle_list, NULL);
>> +	idle_list = NULL;
>> +
>>   free_watch_list:
>>   	free(watch_list);
>>   	watch_list = NULL;
>> @@ -190,6 +199,11 @@ int watch_modify(int fd, uint32_t events, bool force)
>>   	return 0;
>>   }
>> 
>> +static void watch_nop_callback(int fd, uint32_t events, void *user_data)
>> +{
>> +	return;
>> +}
>> +
>>   int watch_remove(int fd)
>>   {
>>   	struct watch_data *data;
>> @@ -212,7 +226,15 @@ int watch_remove(int fd)
>>   	if (data->destroy)
>>   		data->destroy(data->user_data);
>> 
>> -	l_free(data);
>> +	if (epoll_running) {
>> +		/* The epoll events array may point to the same memory as
>> +		 * 'data', so let the event loop free it later
>> +		 */
>> +		data->callback = watch_nop_callback;
>> +		l_queue_push_tail(watch_free_list, data);
>
> Why don't we do this exactly how the idle events are handled.  E.g. set a 
> DESTROYED flag and prune the event list after the epoll processing is 
> complete.
>

In typical use, the idle_list linked list is short or empty and all 
entries are cleaned up on each event loop iteration -- otherwise you're at 
100% CPU. The DESTROYED flag fits well with this, since you plan on 
visiting every entry in the list anyway.

The assumptions around watch_list are the opposite: big list, infrequent 
cleanup. watch_list is an array with 128 entries, and you'd have to check 
all 128 entries on every iteration of the event loop. This could be 
optimized (use a bitmap to flag which ones to free), but there's another 
problem: watch_list is indexed by an fd which was likely closed by the 
data->destroy() callback. It's possible for the fd to get re-used and a 
new watch to be created on that fd even before the data->destroy callback 
returns.


Another proposal: have watch_remove set a global flag that breaks out of 
the inner "for (n = 0; n < nfds; n++)" loop if watch_remove was called, 
and retries the epoll_wait() without processing idle_list. While it's 
pretty simple to implement, there are drawbacks:

1. Could change the event handler sequence depending on how epoll_wait() 
handles event ordering between calls. There don't seem to be any 
guarantees by the syscall with regard to ordering in the returned array. 
The current implementation will handle all existing events before calling 
epoll_wait() again, so there's no chance that events created due to 
actions in one iteration of the event loop will get handled ahead of older 
events. This seems like a useful property to preserve since it prevents 
starvation.

2. The epoll_wait call isn't cheap either, but would only be called one 
extra time for each event involving a watch_remove. The current patch has 
the overhead of one malloc/free pair per removed watch.


Which approach sounds better@this point? Or is there another flavor of 
flag-and-prune that would be workable?

>> +	} else {
>> +		l_free(data);
>> +	}
>>
>>   	return err;
>>   }
>> @@ -352,6 +374,7 @@ LIB_EXPORT int l_main_run(void)
>>
>>   		l_queue_foreach(idle_list, idle_dispatch, NULL);
>>   		l_queue_foreach_remove(idle_list, idle_prune, NULL);
>> +		l_queue_clear(watch_free_list, l_free);
>>   	}
>>
>>   	for (i = 0; i < watch_entries; i++) {
>> @@ -378,6 +401,9 @@ LIB_EXPORT int l_main_run(void)
>>   	l_queue_destroy(idle_list, idle_destroy);
>>   	idle_list = NULL;
>> 
>> +	l_queue_destroy(watch_free_list, l_free);
>> +	watch_free_list = NULL;
>> +
>>   	epoll_running = false;
>>
>>   	close(epoll_fd);
>>


--
Mat Martineau
Intel OTC

Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2451 bytes --]

Hi Mat,

 >>>
>>> +    watch_free_list = l_queue_new();
>>
>> l_queue_new cannot fail.
>
> Ok. I will remove the similar check for idle_list just above this
> (before the patch context) in a separate patch.
>

Thanks.  Some of this code you are touching was created very early in 
ell's lifetime.  So consistency is not quite there yet.

<snip>

>>> -    l_free(data);
>>> +    if (epoll_running) {
>>> +        /* The epoll events array may point to the same memory as
>>> +         * 'data', so let the event loop free it later
>>> +         */
>>> +        data->callback = watch_nop_callback;
>>> +        l_queue_push_tail(watch_free_list, data);
>>
>> Why don't we do this exactly how the idle events are handled.  E.g.
>> set a DESTROYED flag and prune the event list after the epoll
>> processing is complete.
>>
>
> In typical use, the idle_list linked list is short or empty and all
> entries are cleaned up on each event loop iteration -- otherwise you're
> at 100% CPU. The DESTROYED flag fits well with this, since you plan on
> visiting every entry in the list anyway.
>
> The assumptions around watch_list are the opposite: big list, infrequent
> cleanup. watch_list is an array with 128 entries, and you'd have to
> check all 128 entries on every iteration of the event loop. This could

I'm not sure than walking a 128-entry array would be any slower than 
calling malloc/free.  I bet it would be faster.  Would be fun to find out.

> be optimized (use a bitmap to flag which ones to free), but there's
> another problem: watch_list is indexed by an fd which was likely closed
> by the data->destroy() callback. It's possible for the fd to get re-used
> and a new watch to be created on that fd even before the data->destroy
> callback returns.

But then won't you be calling the watch's callback erroneously anyway? 
If this is a concern, then all new watches need to be postponed until 
after the epoll processing has been completed.

>
>
> Another proposal: have watch_remove set a global flag that breaks out of
> the inner "for (n = 0; n < nfds; n++)" loop if watch_remove was called,
> and retries the epoll_wait() without processing idle_list. While it's
> pretty simple to implement, there are drawbacks:

Lets not do this unless there is no better way.  Re-trapping to the 
kernel is way more expensive than anything else we're discussing here.

Regards,
-Denis

Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]


On Wed, 16 Mar 2016, Denis Kenzior wrote:

<snip>

>>>> -    l_free(data);
>>>> +    if (epoll_running) {
>>>> +        /* The epoll events array may point to the same memory as
>>>> +         * 'data', so let the event loop free it later
>>>> +         */
>>>> +        data->callback = watch_nop_callback;
>>>> +        l_queue_push_tail(watch_free_list, data);
>>> 
>>> Why don't we do this exactly how the idle events are handled.  E.g.
>>> set a DESTROYED flag and prune the event list after the epoll
>>> processing is complete.
>>> 
>> 
>> In typical use, the idle_list linked list is short or empty and all
>> entries are cleaned up on each event loop iteration -- otherwise you're
>> at 100% CPU. The DESTROYED flag fits well with this, since you plan on
>> visiting every entry in the list anyway.
>> 
>> The assumptions around watch_list are the opposite: big list, infrequent
>> cleanup. watch_list is an array with 128 entries, and you'd have to
>> check all 128 entries on every iteration of the event loop. This could
>
> I'm not sure than walking a 128-entry array would be any slower than calling 
> malloc/free.  I bet it would be faster.  Would be fun to find out.

Ok, I'm getting out my profiler to have some fun.

>
>> be optimized (use a bitmap to flag which ones to free), but there's
>> another problem: watch_list is indexed by an fd which was likely closed
>> by the data->destroy() callback. It's possible for the fd to get re-used
>> and a new watch to be created on that fd even before the data->destroy
>> callback returns.
>
> But then won't you be calling the watch's callback erroneously anyway? If 
> this is a concern, then all new watches need to be postponed until after the 
> epoll processing has been completed.
>

No, the stale data is a pointer to watch_data not an fd value. By 
deferring the free we can make sure the event loop is calling something 
valid. The contents of watch_data can be reset however we need to. The 
problem is with pointer values stored in watch_list getting clobbered 
before they get used to free memory.

Thinking about this some more, you could work around the issue by keeping 
the fd open, and postpone both the close() and free(). I don't like that 
idea since the application can reasonably need their file/socket/whatever 
to be closed immediately.

All we're searching for is a way to keep a list of pointers until the 
current event loop iteration is done. Either we defer close() and use the 
existing watch_list, or use some low-overhead growable list. I'm assuming 
we don't want to throw an extra 1k of RAM at it to have a static 
watch_cleanup_list.

>> 
>> 
>> Another proposal: have watch_remove set a global flag that breaks out of
>> the inner "for (n = 0; n < nfds; n++)" loop if watch_remove was called,
>> and retries the epoll_wait() without processing idle_list. While it's
>> pretty simple to implement, there are drawbacks:
>
> Lets not do this unless there is no better way.  Re-trapping to the kernel is 
> way more expensive than anything else we're discussing here.

I agree

--
Mat Martineau
Intel OTC

Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

Hi Mat,

 >>> be optimized (use a bitmap to flag which ones to free), but there's
>>> another problem: watch_list is indexed by an fd which was likely closed
>>> by the data->destroy() callback. It's possible for the fd to get re-used
>>> and a new watch to be created on that fd even before the data->destroy
>>> callback returns.
>>
>> But then won't you be calling the watch's callback erroneously anyway?
>> If this is a concern, then all new watches need to be postponed until
>> after the epoll processing has been completed.
>>
>
> No, the stale data is a pointer to watch_data not an fd value. By
> deferring the free we can make sure the event loop is calling something
> valid. The contents of watch_data can be reset however we need to. The
> problem is with pointer values stored in watch_list getting clobbered
> before they get used to free memory.

Now I get it.  I'm good with the proposed solution, but can we avoid 
having dummy callbacks?  Why not simply set the callback to zero (or set 
some flag) and not call it.

Regards,
-Denis