From 420f236d31da76df9982331c596b776ab3d0dd76 Mon Sep 17 00:00:00 2001 From: Robin Jarry Date: Mon, 20 Jun 2022 21:49:05 +0200 Subject: [PATCH] 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 Acked-by: Moritz Poldrack --- worker/imap/fetch.go | 2 +- worker/imap/flags.go | 9 +++-- worker/imap/open.go | 6 +-- worker/imap/seqmap.go | 48 +++++++++++++++++++++++ worker/imap/seqmap_test.go | 78 ++++++++++++++++++++++++++++++++++++++ worker/imap/worker.go | 31 ++++++--------- 6 files changed, 146 insertions(+), 28 deletions(-) create mode 100644 worker/imap/seqmap.go create mode 100644 worker/imap/seqmap_test.go diff --git a/worker/imap/fetch.go b/worker/imap/fetch.go index e8a8251..7d7b72f 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 22c23dd..2bded2a 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 65060fe..a0607d0 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 0000000..2752cc8 --- /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 0000000..0ee0738 --- /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 e890bb8..7debd88 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) } }