imap: fix data race on seqMap array

There are concurrent threads that are accessing and modifying
IMAPWorker.seqMap (the mapping of sequence numbers to message UIDs).
This can lead to crashes when trying to add and remove a message ID.

panic: runtime error: index out of range [391] with length 390

goroutine 1834 [running]:
git.sr.ht/~rjarry/aerc/logging.PanicHandler()
	logging/panic-logger.go:47 +0x6de
panic({0xa41760, 0xc0019b3290})
	/usr/lib/golang/src/runtime/panic.go:838 +0x207
git.sr.ht/~rjarry/aerc/worker/imap.(*IMAPWorker).handleFetchMessages.func1()
	worker/imap/fetch.go:214 +0x185
created by git.sr.ht/~rjarry/aerc/worker/imap.(*IMAPWorker).handleFetchMessages
	worker/imap/fetch.go:209 +0x12b

Use a map which makes more sense than a simple array for random access
operations. Also, it allows better typing for the key values. Protect
the map with a mutex. Add internal API to access the map. Add basic unit
tests to ensure that concurrent access works.

Fixes: https://todo.sr.ht/~rjarry/aerc/49
Signed-off-by: Robin Jarry <robin@jarry.cc>
Acked-by: Moritz Poldrack <moritz@poldrack.dev>

6 files changed, 146 insertions, 28 deletions

diff --git a/worker/imap/fetch.go b/worker/imap/fetch.go
index e8a8251b..7d7b72fe 100644
--- a/worker/imap/fetch.go
+++ b/worker/imap/fetch.go
@@ -211,7 +211,7 @@ func (imapw *IMAPWorker) handleFetchMessages(
 
 		var reterr error
 		for _msg := range messages {
-			imapw.seqMap[_msg.SeqNum-1] = _msg.Uid
+			imapw.seqMap.Put(_msg.SeqNum, _msg.Uid)
 			err := procFunc(_msg)
 			if err != nil {
 				if reterr == nil {
diff --git a/worker/imap/flags.go b/worker/imap/flags.go
index 22c23dd8..2bded2ae 100644
--- a/worker/imap/flags.go
+++ b/worker/imap/flags.go
@@ -2,6 +2,7 @@ package imap
 
 import (
 	"fmt"
+
 	"github.com/emersion/go-imap"
 
 	"git.sr.ht/~rjarry/aerc/logging"
@@ -26,9 +27,11 @@ func (imapw *IMAPWorker) handleDeleteMessages(msg *types.DeleteMessages) {
 		defer logging.PanicHandler()
 
 		for seqNum := range ch {
-			i := seqNum - 1
-			deleted = append(deleted, imapw.seqMap[i])
-			imapw.seqMap = append(imapw.seqMap[:i], imapw.seqMap[i+1:]...)
+			if uid, found := imapw.seqMap.Pop(seqNum); !found {
+				imapw.worker.Logger.Printf("handleDeleteMessages unknown seqnum: %v", seqNum)
+			} else {
+				deleted = append(deleted, uid)
+			}
 		}
 		done <- nil
 	}()
diff --git a/worker/imap/open.go b/worker/imap/open.go
index 65060fe5..a0607d00 100644
--- a/worker/imap/open.go
+++ b/worker/imap/open.go
@@ -61,9 +61,7 @@ func (imapw *IMAPWorker) handleFetchDirectoryContents(
 		}, nil)
 	} else {
 		imapw.worker.Logger.Printf("Found %d UIDs", len(uids))
-		if len(imapw.seqMap) < len(uids) {
-			imapw.seqMap = make([]uint32, len(uids))
-		}
+		imapw.seqMap.Clear()
 		imapw.worker.PostMessage(&types.DirectoryContents{
 			Message: types.RespondTo(msg),
 			Uids:    uids,
@@ -113,7 +111,7 @@ func (imapw *IMAPWorker) handleDirectoryThreaded(
 		aercThreads, count := convertThreads(threads, nil)
 		sort.Sort(types.ByUID(aercThreads))
 		imapw.worker.Logger.Printf("Found %d threaded messages", count)
-		imapw.seqMap = make([]uint32, count)
+		imapw.seqMap.Clear()
 		imapw.worker.PostMessage(&types.DirectoryThreaded{
 			Message: types.RespondTo(msg),
 			Threads: aercThreads,
diff --git a/worker/imap/seqmap.go b/worker/imap/seqmap.go
new file mode 100644
index 00000000..2752cc87
--- /dev/null
+++ b/worker/imap/seqmap.go
@@ -0,0 +1,48 @@
+package imap
+
+import "sync"
+
+type SeqMap struct {
+	lock sync.Mutex
+	// map of IMAP sequence numbers to message UIDs
+	m map[uint32]uint32
+}
+
+func (s *SeqMap) Size() int {
+	s.lock.Lock()
+	size := len(s.m)
+	s.lock.Unlock()
+	return size
+}
+
+func (s *SeqMap) Get(seqnum uint32) (uint32, bool) {
+	s.lock.Lock()
+	uid, found := s.m[seqnum]
+	s.lock.Unlock()
+	return uid, found
+}
+
+func (s *SeqMap) Put(seqnum, uid uint32) {
+	s.lock.Lock()
+	if s.m == nil {
+		s.m = make(map[uint32]uint32)
+	}
+	s.m[seqnum] = uid
+	s.lock.Unlock()
+}
+
+func (s *SeqMap) Pop(seqnum uint32) (uint32, bool) {
+	s.lock.Lock()
+	uid, found := s.m[seqnum]
+	if found {
+		delete(s.m, seqnum)
+	}
+	s.lock.Unlock()
+	return uid, found
+}
+
+func (s *SeqMap) Clear() {
+	s.lock.Lock()
+	s.m = make(map[uint32]uint32)
+	s.lock.Unlock()
+}
diff --git a/worker/imap/seqmap_test.go b/worker/imap/seqmap_test.go
new file mode 100644
index 00000000..0ee07385
--- /dev/null
+++ b/worker/imap/seqmap_test.go
@@ -0,0 +1,78 @@
+package imap
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSeqMap(t *testing.T) {
+	var seqmap SeqMap
+	var uid uint32
+	var found bool
+	assert := assert.New(t)
+
+	assert.Equal(seqmap.Size(), 0)
+
+	_, found = seqmap.Get(42)
+	assert.Equal(found, false)
+
+	_, found = seqmap.Pop(0)
+	assert.Equal(found, false)
+
+	seqmap.Put(1, 1337)
+	seqmap.Put(2, 42)
+	seqmap.Put(3, 1107)
+	assert.Equal(seqmap.Size(), 3)
+
+	_, found = seqmap.Pop(0)
+	assert.Equal(found, false)
+
+	uid, found = seqmap.Get(1)
+	assert.Equal(uid, uint32(1337))
+	assert.Equal(found, true)
+
+	uid, found = seqmap.Pop(1)
+	assert.Equal(uid, uint32(1337))
+	assert.Equal(found, true)
+	assert.Equal(seqmap.Size(), 2)
+
+	_, found = seqmap.Pop(1)
+	assert.Equal(found, false)
+	assert.Equal(seqmap.Size(), 2)
+
+	seqmap.Put(1, 7331)
+	assert.Equal(seqmap.Size(), 3)
+
+	seqmap.Clear()
+	assert.Equal(seqmap.Size(), 0)
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		time.Sleep(20 * time.Millisecond)
+		seqmap.Put(42, 1337)
+		time.Sleep(20 * time.Millisecond)
+		seqmap.Put(43, 1107)
+	}()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for _, found := seqmap.Pop(43); !found; _, found = seqmap.Pop(43) {
+			time.Sleep(1 * time.Millisecond)
+		}
+	}()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for _, found := seqmap.Pop(42); !found; _, found = seqmap.Pop(42) {
+			time.Sleep(1 * time.Millisecond)
+		}
+	}()
+	wg.Wait()
+
+	assert.Equal(seqmap.Size(), 0)
+}
diff --git a/worker/imap/worker.go b/worker/imap/worker.go
index e890bb8d..7debd883 100644
--- a/worker/imap/worker.go
+++ b/worker/imap/worker.go
@@ -61,8 +61,7 @@ type IMAPWorker struct {
 	selected *imap.MailboxStatus
 	updates  chan client.Update
 	worker   *types.Worker
-	// Map of sequence numbers to UIDs, index 0 is seq number 1
-	seqMap []uint32
+	seqMap   SeqMap
 
 	idler    *idler
 	observer *observer
@@ -212,12 +211,6 @@ func (w *IMAPWorker) handleMessage(msg types.WorkerMessage) error {
 
 func (w *IMAPWorker) handleImapUpdate(update client.Update) {
 	w.worker.Logger.Printf("(= %T", update)
-	checkBounds := func(idx, size int) bool {
-		if idx < 0 || idx >= size {
-			return false
-		}
-		return true
-	}
 	switch update := update.(type) {
 	case *client.MailboxUpdate:
 		status := update.Mailbox
@@ -238,11 +231,12 @@ func (w *IMAPWorker) handleImapUpdate(update client.Update) {
 	case *client.MessageUpdate:
 		msg := update.Message
 		if msg.Uid == 0 {
-			if ok := checkBounds(int(msg.SeqNum)-1, len(w.seqMap)); !ok {
-				w.worker.Logger.Println("MessageUpdate error: index out of range")
+			if uid, found := w.seqMap.Get(msg.SeqNum); !found {
+				w.worker.Logger.Printf("MessageUpdate unknown seqnum: %v", msg.SeqNum)
 				return
+			} else {
+				msg.Uid = uid
 			}
-			msg.Uid = w.seqMap[msg.SeqNum-1]
 		}
 		w.worker.PostMessage(&types.MessageInfo{
 			Info: &models.MessageInfo{
@@ -254,16 +248,13 @@ func (w *IMAPWorker) handleImapUpdate(update client.Update) {
 			},
 		}, nil)
 	case *client.ExpungeUpdate:
-		i := update.SeqNum - 1
-		if ok := checkBounds(int(i), len(w.seqMap)); !ok {
-			w.worker.Logger.Println("ExpungeUpdate error: index out of range")
-			return
+		if uid, found := w.seqMap.Pop(update.SeqNum); !found {
+			w.worker.Logger.Printf("ExpungeUpdate unknown seqnum: %v", update.SeqNum)
+		} else {
+			w.worker.PostMessage(&types.MessagesDeleted{
+				Uids: []uint32{uid},
+			}, nil)
 		}
-		uid := w.seqMap[i]
-		w.seqMap = append(w.seqMap[:i], w.seqMap[i+1:]...)
-		w.worker.PostMessage(&types.MessagesDeleted{
-			Uids: []uint32{uid},
-		}, nil)
 	}
 }