From 737d10fac150ba936e422407916655a8630657a5 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 7 Mar 2023 11:18:27 +0100 Subject: [PATCH] BUG/MEDIUM: dns: ensure ring offset is properly reajusted to head Since the below patch, ring offset calculation for readers has changed. commit d9c718863384e32307f65a9ce319dc362b73feb6 MEDIUM: ring: make the offset relative to the head/tail instead of absolute For readers, this requires to adjust their offsets to be relative to the ring head each time read is resumed. Indeed, buffer head can change any time a ring_write() is performed after older entries were purged. This operation was not performed on the DNS code which causes the offset to become invalid. In most cases, the following BUG_ON() was triggered : FATAL: bug condition "msg_len + ofs + cnt + 1 > b_data(buf)" matched at src/dns.c:522 Fix this by adjusting DNS reader offsets when entering dns_session_io_handler() and dns_process_req(). This bug was reproduced by using a backend with 10 servers using SRV record resolution on a single resolvers section. A BUG_ON() crash would occur after less than 5 minutes of process execution. This does not need to be backported as the above patch is not. This should fix github issue #2068. --- src/dns.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/dns.c b/src/dns.c index 003f8b9cfc..1e6f287bb5 100644 --- a/src/dns.c +++ b/src/dns.c @@ -474,9 +474,6 @@ static void dns_session_io_handler(struct appctx *appctx) return; } - - ofs = ds->ofs; - HA_RWLOCK_WRLOCK(DNS_LOCK, &ring->lock); LIST_DEL_INIT(&appctx->wait_entry); HA_RWLOCK_WRUNLOCK(DNS_LOCK, &ring->lock); @@ -491,10 +488,10 @@ static void dns_session_io_handler(struct appctx *appctx) * existing messages before grabbing a reference to a location. This * value cannot be produced after initialization. */ - if (unlikely(ofs == ~0)) { - ofs = 0; + if (unlikely(ds->ofs == ~0)) { + ds->ofs = 0; - HA_ATOMIC_INC(b_peek(buf, ofs)); + HA_ATOMIC_INC(b_peek(buf, ds->ofs)); } /* in this loop, ofs always points to the counter byte that precedes @@ -505,6 +502,10 @@ static void dns_session_io_handler(struct appctx *appctx) /* we were already there, adjust the offset to be relative to * the buffer's head and remove us from the counter. */ + ofs = ds->ofs - b_head_ofs(buf); + if (ds->ofs < b_head_ofs(buf)) + ofs += b_size(buf); + BUG_ON(ofs >= buf->size); HA_ATOMIC_DEC(b_peek(buf, ofs)); @@ -632,7 +633,7 @@ static void dns_session_io_handler(struct appctx *appctx) } HA_ATOMIC_INC(b_peek(buf, ofs)); - ds->ofs = ofs; + ds->ofs = b_peek_ofs(buf, ofs); } HA_RWLOCK_RDUNLOCK(DNS_LOCK, &ring->lock); @@ -1108,8 +1109,6 @@ static struct task *dns_process_req(struct task *t, void *context, unsigned int struct dns_session *ds, *ads; HA_SPIN_LOCK(DNS_LOCK, &dss->lock); - ofs = dss->ofs_req; - HA_RWLOCK_RDLOCK(DNS_LOCK, &ring->lock); /* explanation for the initialization below: it would be better to do @@ -1120,14 +1119,18 @@ static struct task *dns_process_req(struct task *t, void *context, unsigned int * existing messages before grabbing a reference to a location. This * value cannot be produced after initialization. */ - if (unlikely(ofs == ~0)) { - ofs = 0; - HA_ATOMIC_INC(b_peek(buf, ofs)); + if (unlikely(dss->ofs_req == ~0)) { + dss->ofs_req = 0; + HA_ATOMIC_INC(b_peek(buf, dss->ofs_req)); } /* we were already there, adjust the offset to be relative to * the buffer's head and remove us from the counter. */ + ofs = dss->ofs_req - b_head_ofs(buf); + if (dss->ofs_req < b_head_ofs(buf)) + ofs += b_size(buf); + BUG_ON(ofs >= buf->size); HA_ATOMIC_DEC(b_peek(buf, ofs)); @@ -1216,7 +1219,7 @@ static struct task *dns_process_req(struct task *t, void *context, unsigned int } HA_ATOMIC_INC(b_peek(buf, ofs)); - dss->ofs_req = ofs; + dss->ofs_req = b_peek_ofs(buf, ofs); HA_RWLOCK_RDUNLOCK(DNS_LOCK, &ring->lock); -- 2.39.5