From 6a56af2a1486704bae9f38874e1df0001fc3f2d2 Mon Sep 17 00:00:00 2001
From: Agniva De Sarker <agnivade@yahoo.co.in>
Date: Thu, 26 Nov 2020 20:24:23 +0530
Subject: [PATCH] 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>
---
 services/cache/lru.go      | 16 ++++++++--------
 services/cache/lru_test.go | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/services/cache/lru.go b/services/cache/lru.go
index 476a61bc56..de5d88e784 100644
--- a/services/cache/lru.go
+++ b/services/cache/lru.go
@@ -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) {
diff --git a/services/cache/lru_test.go b/services/cache/lru_test.go
index 97e1ca1ea1..641537e97f 100644
--- a/services/cache/lru_test.go
+++ b/services/cache/lru_test.go
@@ -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()
+}