MM-30863: Fix race in LRU (#16382)

* MM-30863: Fix race in LRU

After shortening the critical section, we missed out the fact
that the byte slice is still accessible after the element is returned.
So the lock needs to be active until the byte slice is fully read and
unmarshaled

https://mattermost.atlassian.net/browse/MM-30863

```release-note
NONE
```

* incorporate suggestions

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
This commit is contained in:
Agniva De Sarker
2020-11-26 20:24:23 +05:30
committed by GitHub
co-authored by Mattermod
parent 1e15ad0686
commit 6a56af2a14
2 changed files with 38 additions and 8 deletions
+8 -8
View File
@@ -185,14 +185,14 @@ func (l *LRU) set(key string, value interface{}, ttl time.Duration) error {
}
func (l *LRU) get(key string, value interface{}) error {
e, err := l.getItem(key)
val, err := l.getItem(key)
if err != nil {
return err
}
// We use a fast path for hot structs.
if msgpVal, ok := value.(msgp.Unmarshaler); ok {
_, err := msgpVal.UnmarshalMsg(e.value)
_, err := msgpVal.UnmarshalMsg(val)
return err
}
@@ -207,26 +207,26 @@ func (l *LRU) get(key string, value interface{}) error {
switch v := value.(type) {
case **model.User:
var u model.User
_, err := u.UnmarshalMsg(e.value)
_, err := u.UnmarshalMsg(val)
*v = &u
return err
case **model.Session:
var s model.Session
_, err := s.UnmarshalMsg(e.value)
_, err := s.UnmarshalMsg(val)
*v = &s
return err
case *map[string]*model.User:
var u model.UserMap
_, err := u.UnmarshalMsg(e.value)
_, err := u.UnmarshalMsg(val)
*v = u
return err
}
// Slow path for other structs.
return msgpack.Unmarshal(e.value, value)
return msgpack.Unmarshal(val, value)
}
func (l *LRU) getItem(key string) (*entry, error) {
func (l *LRU) getItem(key string) ([]byte, error) {
l.lock.Lock()
defer l.lock.Unlock()
@@ -240,7 +240,7 @@ func (l *LRU) getItem(key string) (*entry, error) {
return nil, ErrKeyNotFound
}
l.evictList.MoveToFront(ent)
return e, nil
return e.value, nil
}
func (l *LRU) removeElement(e *list.Element) {
+30
View File
@@ -5,6 +5,7 @@ package cache
import (
"fmt"
"sync"
"testing"
"time"
@@ -618,3 +619,32 @@ func BenchmarkLRU(b *testing.B) {
}
})
}
func TestLRURace(t *testing.T) {
l2 := NewLRU(&LRUOptions{
Size: 1,
DefaultExpiry: 0,
InvalidateClusterEvent: "",
})
var wg sync.WaitGroup
l2.Set("test", "value1")
wg.Add(2)
go func() {
defer wg.Done()
value1 := "simplestring"
err := l2.Set("test", value1)
require.Nil(t, err)
}()
go func() {
defer wg.Done()
var val string
err := l2.Get("test", &val)
require.Nil(t, err)
}()
wg.Wait()
}