From 7126394e65547c933741f4772927aefeae257022 Mon Sep 17 00:00:00 2001
From: Juliusz Chroboczek <jch@irif.fr>
Date: Sun, 13 Sep 2020 13:24:05 +0200
Subject: [PATCH] Fix locking in group.go.

Also export some fields as thread-safe methods.
---
 group.go     | 68 +++++++++++++++++++++++++++++++++++++++++-----------
 webclient.go | 18 ++++----------
 webserver.go |  4 ++--
 3 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/group.go b/group.go
index 86a2bd4..c741fae 100644
--- a/group.go
+++ b/group.go
@@ -13,7 +13,6 @@ import (
 	"sort"
 	"strings"
 	"sync"
-	"sync/atomic"
 	"time"
 
 	"github.com/pion/webrtc/v3"
@@ -31,16 +30,47 @@ const (
 )
 
 type group struct {
-	name        string
-	dead        bool
-	description *groupDescription
-	locked      uint32
+	name string
 
-	mu      sync.Mutex
+	mu          sync.Mutex
+	description *groupDescription
+	// indicates that the group no longer exists, but it still has clients
+	dead    bool
+	locked  bool
 	clients map[string]client
 	history []chatHistoryEntry
 }
 
+func (g *group) Locked() bool {
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	return g.locked
+}
+
+func (g *group) SetLocked(locked bool) {
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	g.locked = locked
+}
+
+func (g *group) Public() bool {
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	return g.description.Public
+}
+
+func (g *group) Redirect() string {
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	return g.description.Redirect
+}
+
+func (g *group) AllowRecording() bool {
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	return g.description.AllowRecording
+}
+
 var groups struct {
 	mu     sync.Mutex
 	groups map[string]*group
@@ -94,9 +124,19 @@ func addGroup(name string, desc *groupDescription) (*group, error) {
 			clients:     make(map[string]client),
 		}
 		groups.groups[name] = g
-	} else if desc != nil {
+		return g, nil
+	}
+
+	g.mu.Lock()
+	defer g.mu.Unlock()
+
+	if desc != nil {
 		g.description = desc
-	} else if g.dead || time.Since(g.description.loadTime) > 5*time.Second {
+		g.dead = false
+		return g, nil
+	}
+
+	if g.dead || time.Since(g.description.loadTime) > 5*time.Second {
 		changed, err := descriptionChanged(name, g.description)
 		if err != nil {
 			if !os.IsNotExist(err) {
@@ -176,6 +216,9 @@ func addClient(name string, c client) (*group, error) {
 		return nil, err
 	}
 
+	g.mu.Lock()
+	defer g.mu.Unlock()
+
 	perms, err := getPermission(g.description, c.Credentials())
 	if err != nil {
 		return nil, err
@@ -183,13 +226,10 @@ func addClient(name string, c client) (*group, error) {
 
 	c.SetPermissions(perms)
 
-	if !perms.Op && atomic.LoadUint32(&g.locked) != 0 {
+	if !perms.Op && g.locked {
 		return nil, userError("group is locked")
 	}
 
-	g.mu.Lock()
-	defer g.mu.Unlock()
-
 	if !perms.Op && g.description.MaxClients > 0 {
 		if len(g.clients) >= g.description.MaxClients {
 			return nil, userError("too many users")
@@ -423,8 +463,8 @@ type publicGroup struct {
 
 func getPublicGroups() []publicGroup {
 	gs := make([]publicGroup, 0)
-	rangeGroups(func (g *group) bool {
-		if g.description.Public {
+	rangeGroups(func(g *group) bool {
+		if g.Public() {
 			gs = append(gs, publicGroup{
 				Name:        g.name,
 				ClientCount: len(g.clients),
diff --git a/webclient.go b/webclient.go
index 6ade2b3..5f50531 100644
--- a/webclient.go
+++ b/webclient.go
@@ -12,7 +12,6 @@ import (
 	"os"
 	"strings"
 	"sync"
-	"sync/atomic"
 	"time"
 
 	"sfu/estimator"
@@ -713,10 +712,10 @@ func startClient(conn *websocket.Conn) (err error) {
 		}
 		return
 	}
-	if g.description.Redirect != "" {
+	if redirect := g.Redirect(); redirect != "" {
 		// We normally redirect at the HTTP level, but the group
 		// description could have been edited in the meantime.
-		err = userError("group is now at " + g.description.Redirect)
+		err = userError("group is now at " + redirect)
 		return
 	}
 	c.group = g
@@ -940,10 +939,7 @@ func failConnection(c *webClient, id string, message string) error {
 }
 
 func setPermissions(g *group, id string, perm string) error {
-	g.mu.Lock()
-	defer g.mu.Unlock()
-
-	client := g.getClientUnlocked(id)
+	client := g.getClient(id)
 	if client == nil {
 		return userError("no such user")
 	}
@@ -956,7 +952,7 @@ func setPermissions(g *group, id string, perm string) error {
 	switch perm {
 	case "op":
 		c.permissions.Op = true
-		if g.description.AllowRecording {
+		if g.AllowRecording() {
 			c.permissions.Record = true
 		}
 	case "unop":
@@ -1071,11 +1067,7 @@ func handleClientMessage(c *webClient, m clientMessage) error {
 			if !c.permissions.Op {
 				return c.error(userError("not authorised"))
 			}
-			var locked uint32
-			if m.Kind == "lock" {
-				locked = 1
-			}
-			atomic.StoreUint32(&c.group.locked, locked)
+			c.group.SetLocked(m.Kind == "lock")
 		case "record":
 			if !c.permissions.Record {
 				return c.error(userError("not authorised"))
diff --git a/webserver.go b/webserver.go
index 6ca8050..ac9610b 100644
--- a/webserver.go
+++ b/webserver.go
@@ -151,8 +151,8 @@ func groupHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if g.description.Redirect != "" {
-		http.Redirect(w, r, g.description.Redirect,
+	if redirect := g.Redirect(); redirect != "" {
+		http.Redirect(w, r, redirect,
 			http.StatusPermanentRedirect)
 		return
 	}