From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladislav Shpilevoy Subject: [PATCH 2/2] coio: fix getaddrinfo assertion on 0 timeout Date: Fri, 17 May 2019 18:21:24 +0300 Message-Id: <58bcc084409063f1ee093e436cc06c8f79a47e16.1558106383.git.v.shpilevoy@tarantool.org> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit To: tarantool-patches@freelists.org Cc: vdavydov.dev@gmail.com List-ID: 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)