From f4026d4c1d71c941e8f27c8c8468ace597ac75e7 Mon Sep 17 00:00:00 2001
From: Kegan Dougal <kegan@matrix.org>
Date: Thu, 19 Oct 2023 11:19:28 +0100
Subject: [PATCH] Deep-copy pointer backed fields on room metadata

Fixes #345
---
 internal/roomname.go            | 24 ++++++++++++++++++------
 internal/roomname_test.go       |  2 +-
 sync3/caches/global.go          |  2 +-
 sync3/handler/connstate_live.go |  2 +-
 4 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/internal/roomname.go b/internal/roomname.go
index c000a61..ae83a8b 100644
--- a/internal/roomname.go
+++ b/internal/roomname.go
@@ -60,11 +60,11 @@ func NewRoomMetadata(roomID string) *RoomMetadata {
 	}
 }
 
-// CopyHeroes returns a version of the current RoomMetadata whose Heroes field is
-// a brand-new copy of the original Heroes. The return value's Heroes field can be
+// DeepCopy returns a version of the current RoomMetadata whose Heroes+LatestEventsByType fields are
+// brand-new copies. The return value's Heroes field can be
 // safely modified by the caller, but it is NOT safe for the caller to modify any other
 // fields.
-func (m *RoomMetadata) CopyHeroes() *RoomMetadata {
+func (m *RoomMetadata) DeepCopy() *RoomMetadata {
 	newMetadata := *m
 
 	// XXX: We're doing this because we end up calling RemoveHero() to omit the
@@ -80,9 +80,21 @@ func (m *RoomMetadata) CopyHeroes() *RoomMetadata {
 	newMetadata.Heroes = make([]Hero, len(m.Heroes))
 	copy(newMetadata.Heroes, m.Heroes)
 
-	// ⚠️ NB: there are other pointer fields (e.g. PredecessorRoomID *string) or
-	// and pointer-backed fields (e.g. LatestEventsByType map[string]EventMetadata)
-	// which are not deepcopied here.
+	// copy LatestEventsByType else we risk concurrent map r/w when the connection
+	// reads this map and updates write to it.
+	newMetadata.LatestEventsByType = make(map[string]EventMetadata)
+	for k, v := range m.LatestEventsByType {
+		newMetadata.LatestEventsByType[k] = v
+	}
+
+	newMetadata.ChildSpaceRooms = make(map[string]struct{})
+	for k, v := range m.ChildSpaceRooms {
+		newMetadata.ChildSpaceRooms[k] = v
+	}
+
+	// ⚠️ NB: there are other pointer fields (e.g. PredecessorRoomID *string)
+	// and pointer-backed fields which are not deepcopied here, because they do not
+	// change.
 	return &newMetadata
 }
 
diff --git a/internal/roomname_test.go b/internal/roomname_test.go
index 294c624..c983efc 100644
--- a/internal/roomname_test.go
+++ b/internal/roomname_test.go
@@ -272,7 +272,7 @@ func TestCopyHeroes(t *testing.T) {
 		{ID: chris},
 	}}
 
-	m2 := m1.CopyHeroes()
+	m2 := m1.DeepCopy()
 	// Uncomment this to see why CopyHeroes is necessary!
 	//m2 := m1
 
diff --git a/sync3/caches/global.go b/sync3/caches/global.go
index 5804876..e448c83 100644
--- a/sync3/caches/global.go
+++ b/sync3/caches/global.go
@@ -127,7 +127,7 @@ func (c *GlobalCache) copyRoom(roomID string) *internal.RoomMetadata {
 		logger.Warn().Str("room", roomID).Msg("GlobalCache.LoadRoom: no metadata for this room, returning stub")
 		return internal.NewRoomMetadata(roomID)
 	}
-	return sr.CopyHeroes()
+	return sr.DeepCopy()
 }
 
 // LoadJoinedRooms loads all current joined room metadata for the user given, together
diff --git a/sync3/handler/connstate_live.go b/sync3/handler/connstate_live.go
index e00e62d..fd32078 100644
--- a/sync3/handler/connstate_live.go
+++ b/sync3/handler/connstate_live.go
@@ -344,7 +344,7 @@ func (s *connStateLive) processGlobalUpdates(ctx context.Context, builder *Rooms
 			}
 		}
 
-		metadata := rup.GlobalRoomMetadata().CopyHeroes()
+		metadata := rup.GlobalRoomMetadata().DeepCopy()
 		metadata.RemoveHero(s.userID)
 		// TODO: if we change a room from being a DM to not being a DM, we should call
 		// SetRoom and recalculate avatars. To do that we'd need to