From 223ddedb467494d78ebf67e0ac19206f767ffe6a Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 19 May 2020 19:21:45 +0200 Subject: [PATCH] BUG/MEDIUM: ring: write-lock the ring while attaching/detaching The LIST_ADDQ() and LIST_DEL_INIT() calls made to attach/detach a waiter to the ring were made under a read lock which was sufficient in front of the writer's write lock. But it's not sufficient against other readers! Thus theorically multiple "show events" on the same ring buffer on the CLI could result in a crash, even though for now I couldn't manage to reproduce it. This fixes commit 1d181e489c ("MEDIUM: ring: implement a wait mode for watchers") so it must be backported to 2.1, possibly further if the ring code gets backported. --- src/ring.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/ring.c b/src/ring.c index b94b5699b..ec0656fbc 100644 --- a/src/ring.c +++ b/src/ring.c @@ -248,9 +248,11 @@ int cli_io_handler_show_ring(struct appctx *appctx) if (unlikely(si_ic(si)->flags & (CF_WRITE_ERROR|CF_SHUTW))) return 1; - HA_RWLOCK_RDLOCK(LOGSRV_LOCK, &ring->lock); - + HA_RWLOCK_WRLOCK(LOGSRV_LOCK, &ring->lock); LIST_DEL_INIT(&appctx->ctx.cli.l0); + HA_RWLOCK_WRUNLOCK(LOGSRV_LOCK, &ring->lock); + + HA_RWLOCK_RDLOCK(LOGSRV_LOCK, &ring->lock); /* explanation for the initialization below: it would be better to do * this in the parsing function but this would occasionally result in @@ -321,7 +323,9 @@ int cli_io_handler_show_ring(struct appctx *appctx) */ if (!si_oc(si)->output && !(si_oc(si)->flags & CF_SHUTW)) { /* let's be woken up once new data arrive */ + HA_RWLOCK_WRLOCK(LOGSRV_LOCK, &ring->lock); LIST_ADDQ(&ring->waiters, &appctx->ctx.cli.l0); + HA_RWLOCK_WRUNLOCK(LOGSRV_LOCK, &ring->lock); si_rx_endp_done(si); ret = 0; } @@ -340,7 +344,7 @@ void cli_io_release_show_ring(struct appctx *appctx) if (!ring) return; - HA_RWLOCK_RDLOCK(LOGSRV_LOCK, &ring->lock); + HA_RWLOCK_WRLOCK(LOGSRV_LOCK, &ring->lock); if (ofs != ~0) { /* reader was still attached */ ofs -= ring->ofs; @@ -349,7 +353,7 @@ void cli_io_release_show_ring(struct appctx *appctx) HA_ATOMIC_SUB(b_peek(&ring->buf, ofs), 1); } HA_ATOMIC_SUB(&ring->readers_count, 1); - HA_RWLOCK_RDUNLOCK(LOGSRV_LOCK, &ring->lock); + HA_RWLOCK_WRUNLOCK(LOGSRV_LOCK, &ring->lock); }