From a9e579341181ed877379de022a261da81fc7cf8c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?= <john@erlang.org>
Date: Wed, 27 Jun 2018 14:40:18 +0200
Subject: Fix a race condition when generating async operation ids
The counter used for generating async operation ids was a plain int
shared between all ports, which was incorrect but mostly worked
fine since the ids only had to be unique on a per-port basis.
However, some compilers (notably GCC 8.1.1) generated code that
assumed that this value didn't change between reads. Using a
shortened version of enq_async_w_tmo as an example:
int id = async_ref++;
op->id = id; //A
return id; //B
In GCC 7 and earlier, `async_ref` would be read once and assigned
to `id` before being incremented, which kept the values at A and B
consistent. In GCC 8, `async_ref` was read when assigned at A and
read again at B, and then incremented, which made them inconsistent
if we raced with another port.
This commit fixes the issue by removing `async_ref` altogether and
replacing it with a per-port counter which makes it impossible to
race with someone else.
---
erts/emulator/drivers/common/inet_drv.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/erts/emulator/drivers/common/inet_drv.c b/erts/emulator/drivers/common/inet_drv.c
index ebd13e6f05..3029958f26 100644
--- a/erts/emulator/drivers/common/inet_drv.c
+++ b/erts/emulator/drivers/common/inet_drv.c
@@ -1025,6 +1025,7 @@ typedef struct {
inet_async_op* oph; /* queue head or NULL */
inet_async_op* opt; /* queue tail or NULL */
inet_async_op op_queue[INET_MAX_ASYNC]; /* call queue */
+ int op_ref; /* queue reference generator */
int active; /* 0 = passive, 1 = active, 2 = active once */
Sint16 active_count; /* counter for {active,N} */
@@ -1273,8 +1274,7 @@ static int packet_inet_output(udp_descriptor* udesc, HANDLE event);
/* convert descriptor pointer to inet_descriptor pointer */
#define INETP(d) (&(d)->inet)
-static int async_ref = 0; /* async reference id generator */
-#define NEW_ASYNC_ID() ((async_ref++) & 0xffff)
+#define NEW_ASYNC_ID(desc) ((desc)->op_ref++ & 0xffff)
/* check for transition from active to passive */
#define INET_CHECK_ACTIVE_TO_PASSIVE(inet) \
@@ -1925,7 +1925,7 @@ static void enq_multi_op(tcp_descriptor *desc, char *buf, int req,
ErlDrvTermData caller, MultiTimerData *timeout,
ErlDrvMonitor *monitorp)
{
- int id = NEW_ASYNC_ID();
+ int id = NEW_ASYNC_ID(INETP(desc));
enq_old_multi_op(desc,id,req,caller,timeout,monitorp);
if (buf != NULL)
put_int16(id, buf);
@@ -1994,7 +1994,7 @@ static int remove_multi_op(tcp_descriptor *desc, int *id_p, int *req_p,
static int enq_async_w_tmo(inet_descriptor* desc, char* buf, int req, unsigned timeout,
ErlDrvMonitor *monitorp)
{
- int id = NEW_ASYNC_ID();
+ int id = NEW_ASYNC_ID(desc);
inet_async_op* opp;
if ((opp = desc->oph) == NULL) /* queue empty */
@@ -8310,6 +8310,7 @@ static ErlDrvData inet_start(ErlDrvPort port, int size, int protocol)
desc->delimiter = '\n'; /* line delimiting char */
desc->oph = NULL;
desc->opt = NULL;
+ desc->op_ref = 0;
desc->peer_ptr = NULL;
desc->name_ptr = NULL;
--
cgit v1.2.3