From d00c9d1be5a56fbf990882f9a6da5704b3f1ea75 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 31 May 2017 12:35:45 +0930 Subject: [PATCH 1/1] io: fix nasty io_wake corner case. If we're duplex and one io_always callback makes the other io_always, we screwed up and hit an assertion later when the conn was in the always list but didn't actually want to be. io_wake() uses io_always(), so this is how it happened. Writing a test case for this was a bit fun, too. Signed-off-by: Rusty Russell --- ccan/io/io.c | 15 +++++++- ccan/io/test/run-40-wakeup-mutual.c | 54 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 ccan/io/test/run-40-wakeup-mutual.c diff --git a/ccan/io/io.c b/ccan/io/io.c index f298af70..12b24890 100644 --- a/ccan/io/io.c +++ b/ccan/io/io.c @@ -392,12 +392,25 @@ void io_ready(struct io_conn *conn, int pollflags) void io_do_always(struct io_conn *conn) { + /* There's a corner case where the in next_plan wakes up the + * out, placing it in IO_ALWAYS and we end up processing it immediately, + * only to leave it in the always list. + * + * Yet we can't just process one, in case they are both supposed + * to be done, so grab state beforehand. + */ + bool always_out = (conn->plan[IO_OUT].status == IO_ALWAYS); + if (conn->plan[IO_IN].status == IO_ALWAYS) if (!next_plan(conn, &conn->plan[IO_IN])) return; - if (conn->plan[IO_OUT].status == IO_ALWAYS) + if (always_out) { + /* You can't *unalways* a conn (except by freeing, in which + * case next_plan() returned false */ + assert(conn->plan[IO_OUT].status == IO_ALWAYS); next_plan(conn, &conn->plan[IO_OUT]); + } } void io_do_wakeup(struct io_conn *conn, enum io_direction dir) diff --git a/ccan/io/test/run-40-wakeup-mutual.c b/ccan/io/test/run-40-wakeup-mutual.c new file mode 100644 index 00000000..04ec9442 --- /dev/null +++ b/ccan/io/test/run-40-wakeup-mutual.c @@ -0,0 +1,54 @@ +/* Test previous issue: in duplex case, we wake reader reader wakes writer. */ +#include +/* Include the C files directly. */ +#include +#include +#include +#include +#include + +static struct io_plan *block_reading(struct io_conn *conn, void *unused) +{ + static char buf[1]; + return io_read(conn, buf, sizeof(buf), io_never, NULL); +} + +static struct io_plan *writer_woken(struct io_conn *conn, void *unused) +{ + pass("Writer woken up"); + return io_write(conn, "1", 1, io_close_cb, NULL); +} + +static struct io_plan *reader_woken(struct io_conn *conn, void *unused) +{ + pass("Reader woken up"); + /* Wake writer */ + io_wake(conn); + return block_reading(conn, unused); +} + +static struct io_plan *setup_conn(struct io_conn *conn, void *trigger) +{ + return io_duplex(conn, + io_wait(conn, trigger, reader_woken, NULL), + io_out_wait(conn, conn, writer_woken, NULL)); +} + +int main(void) +{ + int fds[2]; + + plan_tests(3); + ok1(socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) == 0); + + /* We use 'fds' as pointer to wake writer. */ + io_new_conn(NULL, fds[0], setup_conn, fds); + + io_wake(fds); + io_loop(NULL, NULL); + + close(fds[1]); + + /* This exits depending on whether all tests passed */ + return exit_status(); +} -- 2.39.2