[PATCH 0/2] netbox connect timeout assertion

[PATCH 0/2] netbox connect timeout assertion

[Vladislav Shpilevoy]

The patchset combs coio_getaddrinfo() function to make it conform with the Open
Group Standard, and to fix an assertion about 0 timeout.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4209-netbox-connect-timeout-assertion
Issue: https://github.com/tarantool/tarantool/issues/4209

Vladislav Shpilevoy (2):
  coio: make hints in coio_getaddrinfo optional
  coio: fix getaddrinfo assertion on 0 timeout

 src/lib/core/coio_task.c | 34 +++++++++++++++++++---------------
 src/lib/core/coio_task.h | 17 ++++++++++++++---
 src/lib/core/say.c       |  2 +-
 test/unit/coio.cc        | 36 ++++++++++++++++++++++++++++++++++++
 test/unit/coio.result    |  4 ++++
 5 files changed, 74 insertions(+), 19 deletions(-)

-- 
2.20.1 (Apple Git-117)

[PATCH 1/2] coio: make hints in coio_getaddrinfo optional

[Vladislav Shpilevoy]

According to the standard by Open Group, getaddrinfo() hints
argument is optional - it can be NULL. When it is NULL, hints
is assumed to have 0 in ai_flags, ai_socktype, and ai_protocol;
AF_UNSPEC in ai_family.

See The Open Group Base Specifications.
---
 src/lib/core/coio_task.c | 12 ++++++++----
 test/unit/coio.cc        | 18 ++++++++++++++++++
 test/unit/coio.result    |  4 ++++
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c
index 61f376203..d8095eef3 100644
--- a/src/lib/core/coio_task.c
+++ b/src/lib/core/coio_task.c
@@ -378,12 +378,16 @@ coio_getaddrinfo(const char *host, const char *port,
 	 *
 	 * Based on the workaround in https://bugs.python.org/issue17269
 	 */
+	if (hints != NULL) {
 #if defined(__APPLE__) && defined(AI_NUMERICSERV)
-	if (hints && (hints->ai_flags & AI_NUMERICSERV) &&
-	    (port == NULL || (port[0]=='0' && port[1]=='\0'))) port = "00";
+		if ((hints->ai_flags & AI_NUMERICSERV) != 0 &&
+		    (port == NULL || (port[0]=='0' && port[1]=='\0')))
+			port = "00";
 #endif
-	/* Fill hinting information for use by connect(2) or bind(2). */
-	memcpy(&task->hints, hints, sizeof(task->hints));
+		memcpy(&task->hints, hints, sizeof(task->hints));
+	} else {
+		task->hints.ai_family = AF_UNSPEC;
+	}
 	/* make no difference between empty string and NULL for host */
 	if (host != NULL && *host) {
 		task->host = strdup(host);
diff --git a/test/unit/coio.cc b/test/unit/coio.cc
index d1e744508..a2439bf46 100644
--- a/test/unit/coio.cc
+++ b/test/unit/coio.cc
@@ -68,6 +68,22 @@ test_call_f(va_list ap)
 	return res;
 }
 
+static void
+test_getaddrinfo(void)
+{
+	header();
+	plan(1);
+	const char *host = "127.0.0.1";
+	const char *port = "3333";
+	struct addrinfo *i;
+	/* NULL hints should work. It is a standard. */
+	int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
+	is(rc, 0, "getaddringo");
+	freeaddrinfo(i);
+	check_plan();
+	footer();
+}
+
 static int
 main_f(va_list ap)
 {
@@ -86,6 +102,8 @@ main_f(va_list ap)
 	fiber_cancel(call_fiber);
 	fiber_join(call_fiber);
 
+	test_getaddrinfo();
+
 	ev_break(loop(), EVBREAK_ALL);
 	return 0;
 }
diff --git a/test/unit/coio.result b/test/unit/coio.result
index 7d7477979..0373530b0 100644
--- a/test/unit/coio.result
+++ b/test/unit/coio.result
@@ -6,3 +6,7 @@
 	*** test_call_f ***
 # call done with res 0
 	*** test_call_f: done ***
+	*** test_getaddrinfo ***
+1..1
+ok 1 - getaddringo
+	*** test_getaddrinfo: done ***
-- 
2.20.1 (Apple Git-117)

[PATCH 2/2] coio: fix getaddrinfo assertion on 0 timeout

[Vladislav Shpilevoy]

Background. Coio provides a way to schedule arbitrary tasks
execution in worker threads. A task consists of a function to
execute, and a custom destructor.

To push a task the function coio_task_post(task, timeout) was
used. When the function returns 0, a caller can obtain a result
and should free the task manually. But the trick is that if
timeout was 0, the task was posted in a detached state. A
detached task frees its memory automatically despite
coio_task_post() result, and does not even yield. Such a task
object can't be accessed and so much the more freed manually.

coio_getaddrinfo() used coio_task_post() and freed the task when
the latter function returned 0. It led to double free when
timeout was set 0. The bug was introduced here
800cec7311955e1d9dfd2b9f881712d002090f3a in an attempt to do not
yield in say_logrotate, because it is not fiber-safe.

Now there are two functions: coio_task_execute(task, timeout),
which never detaches a task completed successfully, and
coio_task_post(task), which posts a task in a detached state.

Closes #4209
---
 src/lib/core/coio_task.c | 22 +++++++++++-----------
 src/lib/core/coio_task.h | 17 ++++++++++++++---
 src/lib/core/say.c       |  2 +-
 test/unit/coio.cc        | 18 ++++++++++++++++++
 4 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/src/lib/core/coio_task.c b/src/lib/core/coio_task.c
index d8095eef3..908b336ed 100644
--- a/src/lib/core/coio_task.c
+++ b/src/lib/core/coio_task.c
@@ -227,22 +227,22 @@ coio_task_destroy(struct coio_task *task)
 	diag_destroy(&task->diag);
 }
 
+void
+coio_task_post(struct coio_task *task)
+{
+	assert(task->base.type == EIO_CUSTOM);
+	assert(task->fiber == fiber());
+	eio_submit(&task->base);
+	task->fiber = NULL;
+}
+
 int
-coio_task_post(struct coio_task *task, double timeout)
+coio_task_execute(struct coio_task *task, double timeout)
 {
 	assert(task->base.type == EIO_CUSTOM);
 	assert(task->fiber == fiber());
 
 	eio_submit(&task->base);
-	if (timeout == 0) {
-		/*
-		* This is a special case:
-		* we don't wait any response from the task
-		* and just perform just asynchronous post.
-		*/
-		task->fiber = NULL;
-		return 0;
-	}
 	fiber_yield_timeout(timeout);
 	if (!task->complete) {
 		/* timed out or cancelled. */
@@ -409,7 +409,7 @@ coio_getaddrinfo(const char *host, const char *port,
 	}
 
 	/* Post coio task */
-	if (coio_task_post(&task->base, timeout) != 0)
+	if (coio_task_execute(&task->base, timeout) != 0)
 		return -1; /* timed out or cancelled */
 
 	/* Task finished */
diff --git a/src/lib/core/coio_task.h b/src/lib/core/coio_task.h
index bb981f0b0..e81e10cca 100644
--- a/src/lib/core/coio_task.h
+++ b/src/lib/core/coio_task.h
@@ -63,7 +63,11 @@ typedef int (*coio_task_cb)(struct coio_task *task); /* like eio_req */
  */
 struct coio_task {
 	struct eio_req base; /* eio_task - must be first */
-	/** The calling fiber. */
+	/**
+	 * The calling fiber. When set to NULL, the task is
+	 * detached - its resources are freed eventually, and such
+	 * a task should not be accessed after detachment.
+	 */
 	struct fiber *fiber;
 	/** Callbacks. */
 	union {
@@ -102,7 +106,7 @@ void
 coio_task_destroy(struct coio_task *task);
 
 /**
- * Post coio task to EIO thread pool.
+ * Execute a coio task in a worker thread.
  *
  * @param task coio task.
  * @param timeout timeout in seconds.
@@ -114,7 +118,14 @@ coio_task_destroy(struct coio_task *task);
  *            callback.
  */
 int
-coio_task_post(struct coio_task *task, double timeout);
+coio_task_execute(struct coio_task *task, double timeout);
+
+/**
+ * Post a task in detached state. Its result can't be obtained,
+ * and destructor is called after completion.
+ */
+void
+coio_task_post(struct coio_task *task);
 
 /** \cond public */
 
diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 24963cf82..0b2cf2c34 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -343,7 +343,7 @@ say_logrotate(struct ev_loop *loop, struct ev_signal *w, int revents)
 		coio_task_create(&task->base, logrotate_cb, logrotate_cleanup_cb);
 		task->log = log;
 		task->loop = loop();
-		coio_task_post(&task->base, 0);
+		coio_task_post(&task->base);
 	}
 	errno = saved_errno;
 }
diff --git a/test/unit/coio.cc b/test/unit/coio.cc
index a2439bf46..75054d45d 100644
--- a/test/unit/coio.cc
+++ b/test/unit/coio.cc
@@ -80,6 +80,24 @@ test_getaddrinfo(void)
 	int rc = coio_getaddrinfo(host, port, NULL, &i, 1);
 	is(rc, 0, "getaddringo");
 	freeaddrinfo(i);
+
+	/*
+	 * gh-4209: 0 timeout should not be a special value and
+	 * detach a task. Before a fix it led to segfault
+	 * sometimes. The cycle below runs getaddrinfo multiple
+	 * times to increase segfault probability.
+	 */
+	for (int j = 0; j < 5; ++j) {
+		if (coio_getaddrinfo(host, port, NULL, &i, 0) == 0 && i != NULL)
+			freeaddrinfo(i);
+		/*
+		 * Skip one event loop to check, that the coio
+		 * task destructor will not free the memory second
+		 * time.
+		 */
+		fiber_sleep(0);
+	}
+
 	check_plan();
 	footer();
 }
-- 
2.20.1 (Apple Git-117)

Re: [PATCH 0/2] netbox connect timeout assertion

[Vladimir Davydov]

On Fri, May 17, 2019 at 06:21:22PM +0300, Vladislav Shpilevoy wrote:
>   coio: make hints in coio_getaddrinfo optional
>   coio: fix getaddrinfo assertion on 0 timeout

Pushed to master, 2.1, and 1.10.