fix: race condition when drawing group/conference peer list

The number of peers can change between and within iterations leading to an out of bounds index. Now we just lock the entire loop, which is less efficient but necessary.
JFreegman · Jun 24, 2024 · e399f3f · e399f3f
1 parent 8791b47
commit e399f3f
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 20 deletions.
diff --git a/src/conference.c b/src/conference.c
@@ -1067,7 +1067,6 @@ static bool conference_onKey(ToxWindow *self, Toxic *toxic, wint_t key, bool ltr
 
 static void draw_peer(ToxWindow *self, Toxic *toxic, ChatContext *ctx, uint32_t i)
 {
-    pthread_mutex_lock(&Winthread.lock);
     const uint32_t peer_idx = i + conferences[self->num].side_pos;
     const uint32_t peernum = conferences[self->num].name_list[peer_idx].peernum;
     const bool is_self = tox_conference_peer_number_is_ours(toxic->tox, self->num, peernum, NULL);
@@ -1083,26 +1082,20 @@ static void draw_peer(ToxWindow *self, Toxic *toxic, ChatContext *ctx, uint32_t
                           (is_self
                            ? device_is_muted(input, conferences[self->num].audio_in_idx)
                            : peer != NULL && device_is_muted(output, peer->audio_out_idx));
-        pthread_mutex_unlock(&Winthread.lock);
 
         const int aud_attr = A_BOLD | COLOR_PAIR(audio_active && !mute ? GREEN : RED);
         wattron(ctx->sidebar, aud_attr);
         waddch(ctx->sidebar, audio_active ? (mute ? 'M' : '*') : '-');
         wattroff(ctx->sidebar, aud_attr);
         waddch(ctx->sidebar, ' ');
 #endif
-    } else {
-        pthread_mutex_unlock(&Winthread.lock);
     }
 
     /* truncate nick to fit in side panel without modifying list */
     char tmpnick[TOX_MAX_NAME_LENGTH];
     const int maxlen = SIDEBAR_WIDTH - 2 - 2 * audio;
 
-    pthread_mutex_lock(&Winthread.lock);
     memcpy(tmpnick, &conferences[self->num].name_list[peer_idx].name, maxlen);
-    pthread_mutex_unlock(&Winthread.lock);
-
     tmpnick[maxlen] = '\0';
 
     if (is_self) {
@@ -1159,7 +1152,6 @@ static void conference_onDraw(ToxWindow *self, Toxic *toxic)
         wattroff(ctx->sidebar, COLOR_PAIR(PEERLIST_LINE));
 
         pthread_mutex_lock(&Winthread.lock);
-        const uint32_t num_peers = chat->num_peers;
         const bool audio = chat->audio_enabled;
         const int header_lines = sidebar_offset(self->num);
         pthread_mutex_unlock(&Winthread.lock);
@@ -1210,6 +1202,10 @@ static void conference_onDraw(ToxWindow *self, Toxic *toxic)
 #endif  // AUDIO
         }
 
+        pthread_mutex_lock(&Winthread.lock);
+        const uint32_t num_peers = chat->num_peers;
+        pthread_mutex_unlock(&Winthread.lock);
+
         wmove(ctx->sidebar, line, 1);
         wattron(ctx->sidebar, A_BOLD);
         wprintw(ctx->sidebar, "Peers: %"PRIu32"\n", num_peers);
@@ -1221,12 +1217,14 @@ static void conference_onDraw(ToxWindow *self, Toxic *toxic)
         mvwhline(ctx->sidebar, line, 1, ACS_HLINE, SIDEBAR_WIDTH - 1);
         wattroff(ctx->sidebar, COLOR_PAIR(PEERLIST_LINE));
 
-        for (uint32_t i = 0;
-                i < num_peers && i < y2 - header_lines - CHATBOX_HEIGHT;
-                ++i) {
+        pthread_mutex_lock(&Winthread.lock);
+
+        for (uint32_t i = 0; i < chat->num_peers && i < y2 - header_lines - CHATBOX_HEIGHT; ++i) {
             wmove(ctx->sidebar, i + header_lines, 1);
             draw_peer(self, toxic, ctx, i);
         }
+
+        pthread_mutex_unlock(&Winthread.lock);
     }
 
     int y, x;

diff --git a/src/groupchats.c b/src/groupchats.c
@@ -2145,15 +2145,9 @@ static void groupchat_onDraw(ToxWindow *self, Toxic *toxic)
         uint32_t offset = 0;
 
         pthread_mutex_lock(&Winthread.lock);
-        const uint32_t max_idx = chat->max_idx;
-        const uint32_t start = chat->side_pos;
-        pthread_mutex_unlock(&Winthread.lock);
-
-        for (uint32_t i = start; i < max_idx && offset < maxlines; ++i) {
-            pthread_mutex_lock(&Winthread.lock);
 
+        for (uint32_t i = chat->side_pos; i < chat->max_idx && offset < maxlines; ++i) {
             if (!chat->peer_list[i].active) {
-                pthread_mutex_unlock(&Winthread.lock);
                 continue;
             }
 
@@ -2194,8 +2188,6 @@ static void groupchat_onDraw(ToxWindow *self, Toxic *toxic)
                 rolecolour = MAGENTA;
             }
 
-            pthread_mutex_unlock(&Winthread.lock);
-
             if (is_ignored) {
                 wattron(ctx->sidebar, COLOR_PAIR(RED) | A_BOLD);
                 wprintw(ctx->sidebar, "#");
@@ -2212,6 +2204,8 @@ static void groupchat_onDraw(ToxWindow *self, Toxic *toxic)
 
             ++offset;
         }
+
+        pthread_mutex_unlock(&Winthread.lock);
     }
 
     int y;