lby/lk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

kernel/port.c: protect against port_create() race

Browse Source 
In the current design, ports may be created with the same name
because of time-of-check-time-of-use behavior.

This change adds a new port magic, PORTHOLD_MAGIC, which cannot be
accessed using the normal accessors. It is injected into the list
upon a successful, locked search for a name.  It holds the name
until the allocation happens. At which point the port-hold is
deleted and the real port is added while, again, under the thread
lock.

This change assumes using a stack-allocated port is desirable over
preallocating and freeing on each port_create().  If the overhead
of allocation is low, then pre-allocating the new port would be
the cleanest option: (1) allocate, (2) check for collisions,
(3) add to list.

This fix is confirmed by the two_threads_race test completing its
256 iterations without a race. It also fixes the test to expect
ERR_BUSY and to exit cleanly, as it had never completed before.
This commit is contained in:
Will Drewry
2017-12-22 14:57:00 -06:00
parent bc69dededf
commit e08ca47ff1
2 changed files with 50 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
app/tests/port_tests.c
View File

@@ -335,7 +335,12 @@ static int race_thread(void *arg)
}
port_t race_port;
st = port_create("racer_port", PORT_MODE_UNICAST, &race_port);
while(true) {
st = port_create("racer_port", PORT_MODE_UNICAST, &race_port);
if (st != ERR_BUSY)
break;
thread_sleep(25);
} // EINTR all over again . . .
LTRACEF_LEVEL(1, "thread %d: sampling chronochip (%x)\n", tid, race_port);
if (st == ERR_ALREADY_EXISTS) {
// lost the race to create the port.
@@ -344,7 +349,7 @@ static int race_thread(void *arg)
ret = __LINE__;
break;
} else { // Dispose of it now.
thread_sleep(50);
thread_sleep(25);
port_close(race_port);
port_destroy(race_port);
}
@@ -453,7 +458,7 @@ int two_threads_race(void)
// control port says: 0 "REPEAT, or 1 "QUIT"
int ret = 0;
int count = 0;
while (ret == 0 && count++ < 256) {
while (ret == 0) {
LTRACEF_LEVEL(1, "Go!\n");
printf(".");
event_signal(&race_evt, false);
@@ -476,14 +481,18 @@ int two_threads_race(void)
ret = __LINE__;
}
event_unsignal(&race_evt);
LTRACEF_LEVEL(1, "Telling threads to %s\n", (ret == 0 ? "repeat" : "quit"));
st = port_write(w_port, (ret == 0 ? &kRepeat : &kQuit), 1);
int repeat = (ret == 0 && count++ < 99 ? 1 : 0);
LTRACEF_LEVEL(1, "Telling threads to %s\n", (repeat ? "repeat" : "quit"));
st = port_write(w_port, (repeat ? &kRepeat : &kQuit), 1);
if (st < 0) {
printf("could not write port, status = %d\n", st);
ret = __LINE__;
}
if (!repeat) {
break;
}
}
printf("\n");
printf("%d passes completed with result %d\n", count, ret);
st = port_close(r_port0);
if (st < 0) {
@@ -503,12 +512,6 @@ int two_threads_race(void)
ret = __LINE__;
}
st = port_destroy(w_port);
if (st < 0) {
printf("could not destroy port, status = %d\n", st);
ret = __LINE__;
}
int retcode = -1;
thread_join(t1, &retcode, INFINITE_TIME);
if (retcode)
@@ -518,6 +521,14 @@ int two_threads_race(void)
if (retcode)
ret = retcode;
st = port_destroy(w_port);
if (st < 0) {
printf("could not destroy port, status = %d\n", st);
ret = __LINE__;
}
printf("two_thread_race: %d\n", ret);
return ret;

34
kernel/port.c
View File

@@ -45,6 +45,7 @@
#define READPORT_MAGIC (0x70727472) // 'prtr'
#define PORTGROUP_MAGIC (0x70727467) // 'prtg'
#define PORTHOLD_MAGIC (0x70727467) // 'prth'
#define PORT_BUFF_SIZE 8
#define PORT_BUFF_SIZE_BIG 64
@@ -148,16 +149,24 @@ status_t port_create(const char *name, port_mode_t mode, port_t *port)
return ERR_INVALID_ARGS;
}
if (strlen(name) >= PORT_NAME_LEN)
if (strnlen(name, PORT_NAME_LEN) >= PORT_NAME_LEN)
return ERR_INVALID_ARGS;
// Add a stack-allocated port to the list until we can
// replace it with a heap-allocated port.
write_port_t stack_wp = { .magic = PORTHOLD_MAGIC };
// We waste a few cycles here with a throwaway copy.
strlcpy(stack_wp.name, name, sizeof(stack_wp.name));
// lookup for existing port, return that if found.
write_port_t *wp = NULL;
THREAD_LOCK(state1);
list_for_every_entry(&write_port_list, wp, write_port_t, node) {
if (strcmp(wp->name, name) == 0) {
// can't return closed ports.
if (wp->magic == WRITEPORT_MAGIC_X)
// can't return closed or partial ports.
if (wp->magic == WRITEPORT_MAGIC_X ||
wp->magic == PORTHOLD_MAGIC)
wp = NULL;
THREAD_UNLOCK(state1);
if (wp) {
@@ -168,12 +177,17 @@ status_t port_create(const char *name, port_mode_t mode, port_t *port)
}
}
}
list_add_tail(&write_port_list, &stack_wp.node);
THREAD_UNLOCK(state1);
// not found, create the write port and the circular buffer.
wp = calloc(1, sizeof(write_port_t));
if (!wp)
if (!wp) {
THREAD_LOCK(state2);
list_delete(&stack_wp.node);
THREAD_UNLOCK(state2);
return ERR_NO_MEMORY;
}
wp->magic = WRITEPORT_MAGIC_W;
wp->mode = mode;
@@ -184,13 +198,18 @@ status_t port_create(const char *name, port_mode_t mode, port_t *port)
wp->buf = make_buf(size);
if (!wp->buf) {
free(wp);
THREAD_LOCK(state2);
list_delete(&stack_wp.node);
THREAD_UNLOCK(state2);
return ERR_NO_MEMORY;
}
// todo: race condtion! a port with the same name could have been created
// by another thread at is point.
// Avoid a name collision by swapping the temporary placeholder out of the
// list for the actual port.
THREAD_LOCK(state2);
// Let's reserve a stack allocated entry then swap it for the allocated one.
list_add_tail(&write_port_list, &wp->node);
list_delete(&stack_wp.node);
THREAD_UNLOCK(state2);
*port = (void *)wp;
@@ -226,7 +245,8 @@ status_t port_open(const char *name, void *ctx, port_t *port)
THREAD_LOCK(state);
write_port_t *wp = NULL;
list_for_every_entry(&write_port_list, wp, write_port_t, node) {
if (strcmp(wp->name, name) == 0) {
if (strcmp(wp->name, name) == 0 &&
wp->magic != PORTHOLD_MAGIC) {
// found; add read port to write port list.
rp->wport = wp;
if (wp->buf) {