BUG/MAJOR: dns: multi-thread concurrency issue on UDP socket
This patch adds a lock on the struct dgram_conn to ensure
that an other thread cannot trash a fd or alter its status
while the current thread processing it on for send/receive/connect
operations.
Starting with the 2.4 version this could cause a crash when a DNS
request is failing, setting the FD of the dgram structure to -1. If the
dgram structure is reused after that, a read access to fdtab[-1] is
attempted. The crash was only triggered when compiled with ASAN.
In previous versions the concurrency issue also exists but is less
likely to crash.
This patch must be backported until v2.4 and should be
adapt for v < 2.4.
(cherry picked from commit 314e6ec8224e3636d019502286ec46ab418a6e9b)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>
(cherry picked from commit 3add3e1215c042500162df0b0007d7955513b229)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>
diff --git a/src/dns.c b/src/dns.c
index 17fba5f..1ef5e87 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -89,11 +89,15 @@
if (ns->dgram) {
struct dgram_conn *dgram = &ns->dgram->conn;
- int fd = dgram->t.sock.fd;
+ int fd;
- if (dgram->t.sock.fd == -1) {
- if (dns_connect_nameserver(ns) == -1)
+ HA_SPIN_LOCK(DNS_LOCK, &dgram->lock);
+ fd = dgram->t.sock.fd;
+ if (fd == -1) {
+ if (dns_connect_nameserver(ns) == -1) {
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return -1;
+ }
fd = dgram->t.sock.fd;
}
@@ -106,17 +110,21 @@
ret = ring_write(ns->dgram->ring_req, DNS_TCP_MSG_MAX_SIZE, NULL, 0, &myist, 1);
if (!ret) {
ns->counters->snd_error++;
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return -1;
}
fd_cant_send(fd);
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return ret;
}
ns->counters->snd_error++;
fd_delete(fd);
dgram->t.sock.fd = -1;
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return -1;
}
ns->counters->sent++;
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
}
else if (ns->stream) {
struct ist myist;
@@ -147,20 +155,27 @@
if (ns->dgram) {
struct dgram_conn *dgram = &ns->dgram->conn;
- int fd = dgram->t.sock.fd;
+ int fd;
- if (fd == -1)
+ HA_SPIN_LOCK(DNS_LOCK, &dgram->lock);
+ fd = dgram->t.sock.fd;
+ if (fd == -1) {
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return -1;
+ }
if ((ret = recv(fd, data, size, 0)) < 0) {
if (errno == EAGAIN) {
fd_cant_recv(fd);
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return 0;
}
fd_delete(fd);
dgram->t.sock.fd = -1;
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return -1;
}
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
}
else if (ns->stream) {
struct dns_stream_server *dss = ns->stream;
@@ -234,19 +249,26 @@
struct dns_nameserver *ns;
int fd;
+ HA_SPIN_LOCK(DNS_LOCK, &dgram->lock);
+
fd = dgram->t.sock.fd;
/* check if ready for reading */
- if (!fd_recv_ready(fd))
+ if ((fd == -1) || !fd_recv_ready(fd)) {
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return;
+ }
/* no need to go further if we can't retrieve the nameserver */
if ((ns = dgram->owner) == NULL) {
_HA_ATOMIC_AND(&fdtab[fd].state, ~(FD_POLL_HUP|FD_POLL_ERR));
fd_stop_recv(fd);
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return;
}
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
+
ns->process_responses(ns);
}
@@ -260,16 +282,21 @@
uint64_t msg_len;
size_t len, cnt, ofs;
+ HA_SPIN_LOCK(DNS_LOCK, &dgram->lock);
+
fd = dgram->t.sock.fd;
/* check if ready for sending */
- if (!fd_send_ready(fd))
+ if ((fd == -1) || !fd_send_ready(fd)) {
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return;
+ }
/* no need to go further if we can't retrieve the nameserver */
if ((ns = dgram->owner) == NULL) {
_HA_ATOMIC_AND(&fdtab[fd].state, ~(FD_POLL_HUP|FD_POLL_ERR));
fd_stop_send(fd);
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
return;
}
@@ -343,6 +370,7 @@
ofs += ring->ofs;
ns->dgram->ofs_req = ofs;
HA_RWLOCK_RDUNLOCK(DNS_LOCK, &ring->lock);
+ HA_SPIN_UNLOCK(DNS_LOCK, &dgram->lock);
}
@@ -365,6 +393,7 @@
dgram->conn.data = &dns_dgram_cb;
dgram->conn.t.sock.fd = -1;
dgram->conn.addr.to = *sk;
+ HA_SPIN_INIT(&dgram->conn.lock);
ns->dgram = dgram;
dgram->ofs_req = ~0; /* init ring offset */