From 947306a0afb8d830753e842ede8680c45b341c7b Mon Sep 17 00:00:00 2001 From: Azareal Date: Wed, 19 Jun 2019 13:16:03 +1000 Subject: [PATCH] Fix a potential data race when reloading the permissions for a forum. Log more things during start-up. Save a few bytes here and there. Add a couple of initial forum perms store tests. Add an additional group.CanSee test. --- common/forum_perms_store.go | 20 +++++++--- common/group_store.go | 33 +++++++++++------ common/ws_hub.go | 2 + main.go | 5 +++ misc_test.go | 55 +++++++++++++++++++++++++++- templates/topic_alt_quick_reply.html | 2 +- 6 files changed, 96 insertions(+), 21 deletions(-) diff --git a/common/forum_perms_store.go b/common/forum_perms_store.go index 7981dbc5..3ddccbba 100644 --- a/common/forum_perms_store.go +++ b/common/forum_perms_store.go @@ -100,6 +100,7 @@ func (s *MemoryForumPermsStore) Reload(fid int) error { forumPerms[gid] = pperms } DebugLogf("forumPerms: %+v\n", forumPerms) + if fid%2 == 0 { s.evenLock.Lock() s.evenForums[fid] = forumPerms @@ -119,9 +120,15 @@ func (s *MemoryForumPermsStore) Reload(fid int) error { return err } + gcache, ok := Groups.(GroupCache) + if !ok { + TopicListThaw.Thaw() + return nil + } + for _, group := range groups { DebugLogf("Updating the forum permissions for Group #%d", group.ID) - group.CanSee = []int{} + canSee := []int{} for _, fid := range fids { DebugDetailf("Forum #%+v\n", fid) var forumPerms map[int]*ForumPerms @@ -143,23 +150,24 @@ func (s *MemoryForumPermsStore) Reload(fid int) error { forumPerm, ok = forumPerms[group.ID] if !ok { if group.Perms.ViewTopic { - group.CanSee = append(group.CanSee, fid) + canSee = append(canSee, fid) } continue } if forumPerm.Overrides { if forumPerm.ViewTopic { - group.CanSee = append(group.CanSee, fid) + canSee = append(canSee, fid) } } else if group.Perms.ViewTopic { - group.CanSee = append(group.CanSee, fid) + canSee = append(canSee, fid) } DebugDetail("group.ID: ", group.ID) DebugDetailf("forumPerm: %+v\n", forumPerm) - DebugDetail("group.CanSee: ", group.CanSee) + DebugDetail("canSee: ", canSee) } - DebugDetailf("group.CanSee (length %d): %+v \n", len(group.CanSee), group.CanSee) + DebugDetailf("canSee (length %d): %+v \n", len(canSee), canSee) + gcache.SetCanSee(group.ID, canSee) } TopicListThaw.Thaw() return nil diff --git a/common/group_store.go b/common/group_store.go index ee74b287..0f5d7398 100644 --- a/common/group_store.go +++ b/common/group_store.go @@ -31,6 +31,7 @@ type GroupStore interface { type GroupCache interface { CacheSet(group *Group) error + SetCanSee(gid int, canSee []int) error Length() int } @@ -158,12 +159,6 @@ func (s *MemoryGroupStore) Reload(id int) error { return nil } - // TODO: Do we really need this? - /*err = RebuildGroupPermissions(group) - if err != nil { - LogError(err) - return nil - }*/ s.CacheSet(group) TopicListThaw.Thaw() return nil @@ -199,6 +194,21 @@ func (s *MemoryGroupStore) initGroup(group *Group) error { return nil } +func (s *MemoryGroupStore) SetCanSee(gid int, canSee []int) error { + s.Lock() + group, ok := s.groups[gid] + if !ok { + s.Unlock() + return nil + } + ngroup := &Group{} + *ngroup = *group + ngroup.CanSee = canSee + s.groups[group.ID] = ngroup + s.Unlock() + return nil +} + func (s *MemoryGroupStore) CacheSet(group *Group) error { s.Lock() s.groups[group.ID] = group @@ -216,7 +226,7 @@ func (s *MemoryGroupStore) Exists(gid int) bool { // ? Allow two groups with the same name? // TODO: Refactor this -func (mgs *MemoryGroupStore) Create(name string, tag string, isAdmin bool, isMod bool, isBanned bool) (gid int, err error) { +func (s *MemoryGroupStore) Create(name string, tag string, isAdmin bool, isMod bool, isBanned bool) (gid int, err error) { var permstr = "{}" tx, err := qgen.Builder.Begin() if err != nil { @@ -278,7 +288,6 @@ func (mgs *MemoryGroupStore) Create(name string, tag string, isAdmin bool, isMod if err != nil { return 0, err } - err = tx.Commit() if err != nil { return 0, err @@ -289,10 +298,10 @@ func (mgs *MemoryGroupStore) Create(name string, tag string, isAdmin bool, isMod isBanned = false } - mgs.Lock() - mgs.groups[gid] = &Group{gid, name, isMod, isAdmin, isBanned, tag, perms, []byte(permstr), pluginPerms, pluginPermsBytes, blankIntList, 0} - mgs.groupCount++ - mgs.Unlock() + s.Lock() + s.groups[gid] = &Group{gid, name, isMod, isAdmin, isBanned, tag, perms, []byte(permstr), pluginPerms, pluginPermsBytes, blankIntList, 0} + s.groupCount++ + s.Unlock() TopicListThaw.Thaw() return gid, FPStore.ReloadAll() diff --git a/common/ws_hub.go b/common/ws_hub.go index 808e9516..6385867b 100644 --- a/common/ws_hub.go +++ b/common/ws_hub.go @@ -4,6 +4,7 @@ import ( "encoding/json" "sync" "time" + "log" "github.com/gorilla/websocket" ) @@ -38,6 +39,7 @@ func init() { } func (hub *WsHubImpl) Start() { + log.Print("Setting up the WebSocket ticks") ticker := time.NewTicker(time.Minute * 5) defer func() { ticker.Stop() diff --git a/main.go b/main.go index ca074f5a..d43a686c 100644 --- a/main.go +++ b/main.go @@ -52,6 +52,7 @@ func afterDBInit() (err error) { if err != nil { return err } + log.Print("Exitted storeInit") var uids []int tcache := c.Topics.GetCache() @@ -109,6 +110,7 @@ func afterDBInit() (err error) { } } + log.Print("Exitted afterDBInit") return nil } @@ -299,6 +301,8 @@ func storeInit() (err error) { if err != nil { return errors.WithStack(err) } + + log.Print("Initialising the meta store") c.Meta, err = c.NewDefaultMetaStore(acc) if err != nil { return errors.WithStack(err) @@ -514,6 +518,7 @@ func main() { log.Print("Initialising the plugins") c.InitPlugins() + log.Print("Setting up the signal handler") sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) go func() { diff --git a/misc_test.go b/misc_test.go index 6c97469c..8f86b32e 100644 --- a/misc_test.go +++ b/misc_test.go @@ -731,6 +731,56 @@ func TestForumPermsStore(t *testing.T) { if !c.PluginsInited { c.InitPlugins() } + + var initialState = func() { + fid := 1 + gid := 1 + fperms, err := c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,fperms.ViewTopic,"admins should be able to see reports") + + fid = 1 + gid = 2 + fperms, err = c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,fperms.ViewTopic,"mods should be able to see reports") + + fid = 1 + gid = 3 + fperms, err = c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,!fperms.ViewTopic,"members should not be able to see reports") + + fid = 1 + gid = 4 + fperms, err = c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,!fperms.ViewTopic,"banned users should not be able to see reports") + + fid = 2 + gid = 1 + fperms, err = c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,fperms.ViewTopic,"admins should be able to see general") + + fid = 2 + gid = 3 + fperms, err = c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,fperms.ViewTopic,"members should be able to see general") + + fid = 2 + gid = 6 + fperms, err = c.FPStore.Get(fid,gid) + expectNilErr(t,err) + expect(t,fperms.ViewTopic,"guests should be able to see general") + } + initialState() + + expectNilErr(t, c.FPStore.Reload(1)) + initialState() + expectNilErr(t, c.FPStore.Reload(2)) + initialState() } // TODO: Test the group permissions @@ -833,8 +883,9 @@ func TestGroupStore(t *testing.T) { expect(t, !group.IsAdmin, "This shouldn't be an admin group") expect(t, group.IsMod, "This should be a mod group") expect(t, !group.IsBanned, "This shouldn't be a ban group") - expect(t, group.CanSee!=nil, "group.CanSee must not be nil") - expect(t, len(group.CanSee) > 0, "group.CanSee should not be zero") + expect(t, group.CanSee != nil, "group.CanSee must not be nil") + expect(t, len(group.CanSee) == 1, "len(group.CanSee) should not be one") + expect(t, group.CanSee[0] == 2, "group.CanSee[0] should be 2") canSee := group.CanSee // Make sure the data is static diff --git a/templates/topic_alt_quick_reply.html b/templates/topic_alt_quick_reply.html index ce8372e4..754ef0b3 100644 --- a/templates/topic_alt_quick_reply.html +++ b/templates/topic_alt_quick_reply.html @@ -9,7 +9,7 @@
- +