[MM-21378] Add mutex to model.Post to guard against race conditions on Post.Props (#13884)

* Add mutex to model.Post to guard against race conditions on Post.Props * Rename mutex * Add GetProp() method to Post * Fix more tests * Fix flaky test Benchmarks: BenchmarkPostPropsGet_indirect BenchmarkPostPropsGet_indirect-2 85026746 13.0 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_indirect-4 90273747 13.0 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_indirect-8 88324293 13.0 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_indirect-16 91427720 13.1 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_direct BenchmarkPostPropsGet_direct-2 1000000000 0.242 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_direct-4 1000000000 0.241 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_direct-8 1000000000 0.240 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsGet_direct-16 1000000000 0.241 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsAdd_indirect BenchmarkPostPropsAdd_indirect-2 5602224 203 ns/op 336 B/op 2 allocs/op BenchmarkPostPropsAdd_indirect-4 5959496 206 ns/op 336 B/op 2 allocs/op BenchmarkPostPropsAdd_indirect-8 5833999 205 ns/op 336 B/op 2 allocs/op BenchmarkPostPropsAdd_indirect-16 5802493 225 ns/op 336 B/op 2 allocs/op BenchmarkPostPropsAdd_direct BenchmarkPostPropsAdd_direct-2 100000000 11.3 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsAdd_direct-4 100000000 11.3 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsAdd_direct-8 100000000 11.6 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsAdd_direct-16 99840794 11.4 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsDel_indirect BenchmarkPostPropsDel_indirect-2 18824002 61.9 ns/op 48 B/op 1 allocs/op BenchmarkPostPropsDel_indirect-4 19470736 63.8 ns/op 48 B/op 1 allocs/op BenchmarkPostPropsDel_indirect-8 17640460 65.3 ns/op 48 B/op 1 allocs/op BenchmarkPostPropsDel_indirect-16 18692962 65.4 ns/op 48 B/op 1 allocs/op BenchmarkPostPropsDel_direct BenchmarkPostPropsDel_direct-2 516257440 2.34 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsDel_direct-4 514865216 2.43 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsDel_direct-8 511330477 2.37 ns/op 0 B/op 0 allocs/op BenchmarkPostPropsDel_direct-16 499504010 2.38 ns/op 0 B/op 0 allocs/op
2020-03-13 21:12:20 +01:00
--- a/model/integration_action.go
+++ b/model/integration_action.go
@@ -368,8 +368,8 @@ func (r *SubmitDialogResponse) ToJson() []byte {

 func (o *Post) StripActionIntegrations() {
 	attachments := o.Attachments()
-	if o.Props["attachments"] != nil {
-		o.Props["attachments"] = attachments
+	if o.GetProp("attachments") != nil {
+		o.AddProp("attachments", attachments)
 	}
 	for _, attachment := range attachments {
 		for _, action := range attachment.Actions {
@@ -390,10 +390,10 @@ func (o *Post) GetAction(id string) *PostAction {
 }

 func (o *Post) GenerateActionIds() {
-	if o.Props["attachments"] != nil {
-		o.Props["attachments"] = o.Attachments()
+	if o.GetProp("attachments") != nil {
+		o.AddProp("attachments", o.Attachments())
 	}
-	if attachments, ok := o.Props["attachments"].([]*SlackAttachment); ok {
+	if attachments, ok := o.GetProp("attachments").([]*SlackAttachment); ok {
 		for _, attachment := range attachments {
 			for _, action := range attachment.Actions {
 				if action.Id == "" {
@@ -411,7 +411,7 @@ func AddPostActionCookies(o *Post, secret []byte) *Post {
 	retainProps := map[string]interface{}{}
 	removeProps := []string{}
 	for _, key := range PostActionRetainPropKeys {
-		value, ok := p.Props[key]
+		value, ok := p.GetProps()[key]
 		if ok {
 			retainProps[key] = value
 		} else {
--- a/model/post.go
+++ b/model/post.go
@@ -5,11 +5,13 @@ package model

 import (
 	"encoding/json"
+	"errors"
 	"io"
 	"net/http"
 	"regexp"
 	"sort"
 	"strings"
+	"sync"
 	"unicode/utf8"

 	"github.com/mattermost/mattermost-server/v5/utils/markdown"
@@ -83,7 +85,8 @@ type Post struct {
 	MessageSource string `json:"message_source,omitempty" db:"-"`

 	Type          string          `json:"type"`
-	Props         StringInterface `json:"props"`
+	propsMu       sync.RWMutex    `db:"-"`       // Unexported mutex used to guard Post.Props.
+	Props         StringInterface `json:"props"` // Deprecated: use GetProps()
 	Hashtags      string          `json:"hashtags"`
 	Filenames     StringArray     `json:"filenames,omitempty"` // Deprecated, do not use this field any more
 	FileIds       StringArray     `json:"file_ids,omitempty"`
@@ -156,10 +159,46 @@ type PostForIndexing struct {
 	ParentCreateAt *int64 `json:"parent_create_at"`
 }

-// Clone shallowly copies the post.
+// ShallowCopy is an utility function to shallow copy a Post to the given
+// destination without touching the internal RWMutex.
+func (o *Post) ShallowCopy(dst *Post) error {
+	if dst == nil {
+		return errors.New("dst cannot be nil")
+	}
+	o.propsMu.RLock()
+	defer o.propsMu.RUnlock()
+	dst.propsMu.Lock()
+	defer dst.propsMu.Unlock()
+	dst.Id = o.Id
+	dst.CreateAt = o.CreateAt
+	dst.UpdateAt = o.UpdateAt
+	dst.EditAt = o.EditAt
+	dst.DeleteAt = o.DeleteAt
+	dst.IsPinned = o.IsPinned
+	dst.UserId = o.UserId
+	dst.ChannelId = o.ChannelId
+	dst.RootId = o.RootId
+	dst.ParentId = o.ParentId
+	dst.OriginalId = o.OriginalId
+	dst.Message = o.Message
+	dst.MessageSource = o.MessageSource
+	dst.Type = o.Type
+	dst.Props = o.Props
+	dst.Hashtags = o.Hashtags
+	dst.Filenames = o.Filenames
+	dst.FileIds = o.FileIds
+	dst.PendingPostId = o.PendingPostId
+	dst.HasReactions = o.HasReactions
+	dst.ReplyCount = o.ReplyCount
+	dst.Metadata = o.Metadata
+	return nil
+}
+
+// Clone shallowly copies the post and returns the copy.
 func (o *Post) Clone() *Post {
-	copy := *o
-	return &copy
+	copy := &Post{}
+	o.ShallowCopy(copy)
+	return copy
 }

 func (o *Post) ToJson() string {
@@ -199,7 +238,6 @@ func (o *Post) Etag() string {
 }

 func (o *Post) IsValid(maxPostSize int) *AppError {
-
 	if len(o.Id) != 26 {
 		return NewAppError("Post.IsValid", "model.post.is_valid.id.app_error", nil, "", http.StatusBadRequest)
 	}
@@ -286,7 +324,7 @@ func (o *Post) IsValid(maxPostSize int) *AppError {
 		return NewAppError("Post.IsValid", "model.post.is_valid.file_ids.app_error", nil, "id="+o.Id, http.StatusBadRequest)
 	}

-	if utf8.RuneCountInString(StringInterfaceToJson(o.Props)) > POST_PROPS_MAX_RUNES {
+	if utf8.RuneCountInString(StringInterfaceToJson(o.GetProps())) > POST_PROPS_MAX_RUNES {
 		return NewAppError("Post.IsValid", "model.post.is_valid.props.app_error", nil, "id="+o.Id, http.StatusBadRequest)
 	}

@@ -299,8 +337,8 @@ func (o *Post) SanitizeProps() {
 	}

 	for _, member := range membersToSanitize {
-		if _, ok := o.Props[member]; ok {
-			delete(o.Props, member)
+		if _, ok := o.GetProps()[member]; ok {
+			o.DelProp(member)
 		}
 	}
 }
@@ -321,8 +359,8 @@ func (o *Post) PreSave() {
 }

 func (o *Post) PreCommit() {
-	if o.Props == nil {
-		o.Props = make(map[string]interface{})
+	if o.GetProps() == nil {
+		o.SetProps(make(map[string]interface{}))
 	}

 	if o.Filenames == nil {
@@ -340,16 +378,49 @@ func (o *Post) PreCommit() {
 }

 func (o *Post) MakeNonNil() {
-	if o.Props == nil {
-		o.Props = make(map[string]interface{})
+	if o.GetProps() == nil {
+		o.SetProps(make(map[string]interface{}))
 	}
 }

+func (o *Post) DelProp(key string) {
+	o.propsMu.Lock()
+	defer o.propsMu.Unlock()
+	propsCopy := make(map[string]interface{}, len(o.Props)-1)
+	for k, v := range o.Props {
+		propsCopy[k] = v
+	}
+	delete(propsCopy, key)
+	o.Props = propsCopy
+}
+
 func (o *Post) AddProp(key string, value interface{}) {
+	o.propsMu.Lock()
+	defer o.propsMu.Unlock()
+	propsCopy := make(map[string]interface{}, len(o.Props)+1)
+	for k, v := range o.Props {
+		propsCopy[k] = v
+	}
+	propsCopy[key] = value
+	o.Props = propsCopy
+}

-	o.MakeNonNil()
+func (o *Post) GetProps() StringInterface {
+	o.propsMu.RLock()
+	defer o.propsMu.RUnlock()
+	return o.Props
+}

-	o.Props[key] = value
+func (o *Post) SetProps(props StringInterface) {
+	o.propsMu.Lock()
+	defer o.propsMu.Unlock()
+	o.Props = props
+}
+
+func (o *Post) GetProp(key string) interface{} {
+	o.propsMu.RLock()
+	defer o.propsMu.RUnlock()
+	return o.Props[key]
 }

 func (o *Post) IsSystemMessage() bool {
@@ -379,7 +450,8 @@ func (o *Post) Patch(patch *PostPatch) {
 	}

 	if patch.Props != nil {
-		o.Props = *patch.Props
+		newProps := *patch.Props
+		o.SetProps(newProps)
 	}

 	if patch.FileIds != nil {
@@ -464,11 +536,11 @@ func findAtChannelMention(message string) (mention string, found bool) {
 }

 func (o *Post) Attachments() []*SlackAttachment {
-	if attachments, ok := o.Props["attachments"].([]*SlackAttachment); ok {
+	if attachments, ok := o.GetProp("attachments").([]*SlackAttachment); ok {
 		return attachments
 	}
 	var ret []*SlackAttachment
-	if attachments, ok := o.Props["attachments"].([]interface{}); ok {
+	if attachments, ok := o.GetProp("attachments").([]interface{}); ok {
 		for _, attachment := range attachments {
 			if enc, err := json.Marshal(attachment); err == nil {
 				var decoded SlackAttachment
--- a/model/post_list.go
+++ b/model/post_list.go
@@ -46,9 +46,9 @@ func (o *PostList) StripActionIntegrations() {
 	posts := o.Posts
 	o.Posts = make(map[string]*Post)
 	for id, post := range posts {
-		pcopy := *post
+		pcopy := post.Clone()
 		pcopy.StripActionIntegrations()
-		o.Posts[id] = &pcopy
+		o.Posts[id] = pcopy
 	}
 }

--- a/model/post_test.go
+++ b/model/post_test.go
@@ -6,6 +6,7 @@ package model
 import (
 	"io/ioutil"
 	"strings"
+	"sync"
 	"testing"

 	"github.com/stretchr/testify/assert"
@@ -18,7 +19,7 @@ func TestPostToJson(t *testing.T) {
 	ro := PostFromJson(strings.NewReader(j))

 	assert.NotNil(t, ro)
-	assert.Equal(t, o, *ro)
+	assert.Equal(t, &o, ro.Clone())
 }

 func TestPostFromJsonError(t *testing.T) {
@@ -124,7 +125,7 @@ func TestPostSanitizeProps(t *testing.T) {

 	post1.SanitizeProps()

-	require.Nil(t, post1.Props[PROPS_ADD_CHANNEL_MEMBER])
+	require.Nil(t, post1.GetProp(PROPS_ADD_CHANNEL_MEMBER))

 	post2 := &Post{
 		Message: "test",
@@ -135,7 +136,7 @@ func TestPostSanitizeProps(t *testing.T) {

 	post2.SanitizeProps()

-	require.Nil(t, post2.Props[PROPS_ADD_CHANNEL_MEMBER])
+	require.Nil(t, post2.GetProp(PROPS_ADD_CHANNEL_MEMBER))

 	post3 := &Post{
 		Message: "test",
@@ -147,9 +148,9 @@ func TestPostSanitizeProps(t *testing.T) {

 	post3.SanitizeProps()

-	require.Nil(t, post3.Props[PROPS_ADD_CHANNEL_MEMBER])
+	require.Nil(t, post3.GetProp(PROPS_ADD_CHANNEL_MEMBER))

-	require.NotNil(t, post3.Props["attachments"])
+	require.NotNil(t, post3.GetProp("attachments"))
 }

 func TestPost_AttachmentsEqual(t *testing.T) {
@@ -502,6 +503,182 @@ func BenchmarkRewriteImageURLs(b *testing.B) {
 		})
 	}
 }
+
+func TestPostShallowCopy(t *testing.T) {
+	var dst *Post
+	p := &Post{
+		Id: NewId(),
+	}
+
+	err := p.ShallowCopy(dst)
+	require.Error(t, err)
+
+	dst = &Post{}
+	err = p.ShallowCopy(dst)
+	require.NoError(t, err)
+	require.Equal(t, p, dst)
+	require.Condition(t, func() bool {
+		return p != dst
+	})
+}
+
+func TestPostClone(t *testing.T) {
+	p := &Post{
+		Id: NewId(),
+	}
+
+	pp := p.Clone()
+	require.Equal(t, p, pp)
+	require.Condition(t, func() bool {
+		return p != pp
+	})
+	require.Condition(t, func() bool {
+		return &p.propsMu != &pp.propsMu
+	})
+}
+
+func BenchmarkClonePost(b *testing.B) {
+	p := Post{}
+	for i := 0; i < b.N; i++ {
+		_ = p.Clone()
+	}
+}
+
+func BenchmarkPostPropsGet_indirect(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	for i := 0; i < b.N; i++ {
+		_ = p.GetProps()
+	}
+}
+
+func BenchmarkPostPropsGet_direct(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	for i := 0; i < b.N; i++ {
+		_ = p.Props
+	}
+}
+
+func BenchmarkPostPropsAdd_indirect(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	for i := 0; i < b.N; i++ {
+		p.AddProp("test", "somevalue")
+	}
+}
+
+func BenchmarkPostPropsAdd_direct(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	for i := 0; i < b.N; i++ {
+		p.Props["test"] = "somevalue"
+	}
+}
+
+func BenchmarkPostPropsDel_indirect(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	p.AddProp("test", "somevalue")
+	for i := 0; i < b.N; i++ {
+		p.DelProp("test")
+	}
+}
+
+func BenchmarkPostPropsDel_direct(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	for i := 0; i < b.N; i++ {
+		delete(p.Props, "test")
+	}
+}
+
+func BenchmarkPostPropGet_direct(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	p.Props["somekey"] = "somevalue"
+	for i := 0; i < b.N; i++ {
+		_ = p.Props["somekey"]
+	}
+}
+
+func BenchmarkPostPropGet_indirect(b *testing.B) {
+	p := Post{
+		Props: make(StringInterface),
+	}
+	p.Props["somekey"] = "somevalue"
+	for i := 0; i < b.N; i++ {
+		_ = p.GetProp("somekey")
+	}
+}
+
+// TestPostPropsDataRace tries to trigger data race conditions related to Post.Props.
+// It's meant to be run with the -race flag.
+func TestPostPropsDataRace(t *testing.T) {
+	p := Post{Message: "test"}
+
+	wg := sync.WaitGroup{}
+	wg.Add(7)
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			p.AddProp("test", "test")
+		}
+		wg.Done()
+	}()
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			_ = p.GetProp("test")
+		}
+		wg.Done()
+	}()
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			p.AddProp("test", "test2")
+		}
+		wg.Done()
+	}()
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			_ = p.GetProps()["test"]
+		}
+		wg.Done()
+	}()
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			p.DelProp("test")
+		}
+		wg.Done()
+	}()
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			p.SetProps(make(StringInterface))
+		}
+		wg.Done()
+	}()
+
+	go func() {
+		for i := 0; i < 100; i++ {
+			_ = p.Clone()
+		}
+		wg.Done()
+	}()
+
+	wg.Wait()
+}
+
 func Test_findAtChannelMention(t *testing.T) {
 	testCases := []struct {
 		Name    string