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>
This commit is contained in:
Robin Jarry 2022-06-20 21:49:05 +02:00
parent 389c0f82a7
commit 420f236d31
6 changed files with 146 additions and 28 deletions

View File

@ -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 {

View File

@ -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
}()

View File

@ -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,

48
worker/imap/seqmap.go Normal file
View File

@ -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()
}

View File

@ -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)
}

View File

@ -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,17 +248,14 @@ 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
}
uid := w.seqMap[i]
w.seqMap = append(w.seqMap[:i], w.seqMap[i+1:]...)
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)
}
}
}
func (w *IMAPWorker) Run() {