Skip to content

Commit

Permalink
vici: Don't lock connection in write mode when enabling on_write() ca…
Browse files Browse the repository at this point in the history
…llback

This should prevent a deadlock that could previously be caused when a
control-log event was raised.  The deadlock looked something like this:

 * Thread A holds the read lock on bus_t and raises the control-log event.
   This requires acquiring the connection entry in write mode to queue the
   outgoing message.  If it is already held by another thread, this blocks
   on a condvar.

 * Thread B is registering the on_write() callback on the same connection's
   stream due to a previous log message.  Before, the code acquired the
   entry in write mode, thus, blocking thread A.  To remove/add the stream
   the mutex in watcher_t needs to be acquired.

 * Thread C is in watcher_t's watch() and holds the mutex while logging on
   level 2 or 3.  The latter requires the read lock on bus_t, which should
   usually be acquirable unless writers are waiting on the lock and the
   implementation currently blocks new readers.

 * Thread D is removing a logger from the bus (e.g. after an initiate()
   call) and is waiting to acquire the write lock on bus_t and thereby
   blocking thread C.

With this change, thread B should not block thread A anymore.  So thread D
and thread C should eventually be able to proceed as well.

Thread A could be held up a bit if there is a thread already sending
messages for the same connection, but that should only cause a delay, no
deadlock, as on_write() and do_write() don't log anything while keeping
the entry locked in write mode.
  • Loading branch information
tobiasbrunner committed Apr 13, 2023
1 parent 5284cec commit f33cf93
Showing 1 changed file with 10 additions and 3 deletions.
13 changes: 10 additions & 3 deletions src/libcharon/plugins/vici/vici_socket.c
Expand Up @@ -127,6 +127,8 @@ typedef struct {
int readers;
/** any users writing over this connection? */
int writers;
/** any users using this connection at all? */
int users;
/** condvar to wait for usage */
condvar_t *cond;
} entry_t;
Expand Down Expand Up @@ -211,6 +213,7 @@ static entry_t* find_entry(private_vici_socket_t *this, stream_t *stream,
{
entry->writers++;
}
entry->users++;
found = entry;
break;
}
Expand Down Expand Up @@ -240,7 +243,7 @@ static entry_t* remove_entry(private_vici_socket_t *this, u_int id)
if (entry->id == id)
{
candidate = TRUE;
if (entry->readers || entry->writers)
if (entry->readers || entry->writers || entry->users)
{
entry->cond->wait(entry->cond, this->mutex);
break;
Expand Down Expand Up @@ -273,6 +276,7 @@ static void put_entry(private_vici_socket_t *this, entry_t *entry,
{
entry->writers--;
}
entry->users--;
entry->cond->signal(entry->cond);
this->mutex->unlock(this->mutex);
}
Expand Down Expand Up @@ -583,6 +587,7 @@ CALLBACK(on_accept, bool,
.queue = array_create(sizeof(chunk_t), 0),
.cond = condvar_create(CONDVAR_TYPE_DEFAULT),
.readers = 1,
.users = 1,
);

this->mutex->lock(this->mutex);
Expand All @@ -606,11 +611,13 @@ CALLBACK(enable_writer, job_requeue_t,
{
entry_t *entry;

entry = find_entry(sel->this, NULL, sel->id, FALSE, TRUE);
/* we don't modify the in- or outbound queue, so don't lock the entry in
* reader or writer mode */
entry = find_entry(sel->this, NULL, sel->id, FALSE, FALSE);
if (entry)
{
entry->stream->on_write(entry->stream, on_write, sel->this);
put_entry(sel->this, entry, FALSE, TRUE);
put_entry(sel->this, entry, FALSE, FALSE);
}
return JOB_REQUEUE_NONE;
}
Expand Down

0 comments on commit f33cf93

Please sign in to comment.