fixed the race condition that led to concurrent map access in a publi…

…cshare manager
2403905 · Jan 23, 2024 · 4c4dcd4 · 4c4dcd4
1 parent e8fc07f
commit 4c4dcd4
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 16 deletions.
diff --git a/changelog/unreleased/fix-concurrent-access-to-map.md b/changelog/unreleased/fix-concurrent-access-to-map.md
@@ -0,0 +1,6 @@
+Bugfix: Fix concurrent access to a map
+
+We fixed the race condition that led to concurrent map access in a publicshare manager.
+
+https://github.com/cs3org/reva/pull/4472
+https://github.com/owncloud/ocis/issues/8255
diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go
@@ -219,6 +219,9 @@ func (m *manager) Dump(ctx context.Context, shareChan chan<- *publicshare.WithPa
 
 // Load imports public shares and received shares from channels (e.g. during migration)
 func (m *manager) Load(ctx context.Context, shareChan <-chan *publicshare.WithPassword) error {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+
 	db, err := m.persistence.Read(ctx)
 	if err != nil {
 		return err
@@ -414,6 +417,9 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link
 
 // GetPublicShare gets a public share either by ID or Token.
 func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.PublicShareReference, sign bool) (*link.PublicShare, error) {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+
 	if ref.GetToken() != "" {
 		ps, pw, err := m.getByToken(ctx, ref.GetToken())
 		if err != nil {
@@ -428,9 +434,6 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu
 		return ps, nil
 	}
 
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
-
 	db, err := m.persistence.Read(ctx)
 	if err != nil {
 		return nil, err
@@ -566,15 +569,13 @@ func (m *manager) cleanupExpiredShares() {
 	}
 }
 
+// revokeExpiredPublicShare doesn't have a lock inside, ensure a lock before call
 func (m *manager) revokeExpiredPublicShare(ctx context.Context, s *link.PublicShare, u *user.User) error {
 	if !m.enableExpiredSharesCleanup {
 		return nil
 	}
 
-	m.mutex.Unlock()
-	defer m.mutex.Lock()
-
-	err := m.RevokePublicShare(ctx, u, &link.PublicShareReference{
+	err := m.revokePublicShare(ctx, u, &link.PublicShareReference{
 		Spec: &link.PublicShareReference_Id{
 			Id: &link.PublicShareId{
 				OpaqueId: s.Id.OpaqueId,
@@ -592,11 +593,16 @@ func (m *manager) revokeExpiredPublicShare(ctx context.Context, s *link.PublicSh
 // RevokePublicShare undocumented.
 func (m *manager) RevokePublicShare(ctx context.Context, u *user.User, ref *link.PublicShareReference) error {
 	m.mutex.Lock()
+	defer m.mutex.Unlock()
+	return m.revokePublicShare(ctx, u, ref)
+}
+
+// revokePublicShare doesn't have a lock inside, ensure a lock before call
+func (m *manager) revokePublicShare(ctx context.Context, u *user.User, ref *link.PublicShareReference) error {
 	db, err := m.persistence.Read(ctx)
 	if err != nil {
 		return err
 	}
-	m.mutex.Unlock()
 
 	switch {
 	case ref.GetId() != nil && ref.GetId().OpaqueId != "":
@@ -615,20 +621,16 @@ func (m *manager) RevokePublicShare(ctx context.Context, u *user.User, ref *link
 		return errors.New("reference does not exist")
 	}
 
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
 	return m.persistence.Write(ctx, db)
 }
 
+// getByToken doesn't have a lock inside, ensure a lock before call
 func (m *manager) getByToken(ctx context.Context, token string) (*link.PublicShare, string, error) {
 	db, err := m.persistence.Read(ctx)
 	if err != nil {
 		return nil, "", err
 	}
 
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
-
 	for _, v := range db {
 		var local link.PublicShare
 		if err := utils.UnmarshalJSONToProtoV1([]byte(v.(map[string]interface{})["share"].(string)), &local); err != nil {
@@ -646,14 +648,14 @@ func (m *manager) getByToken(ctx context.Context, token string) (*link.PublicSha
 
 // GetPublicShareByToken gets a public share by its opaque token.
 func (m *manager) GetPublicShareByToken(ctx context.Context, token string, auth *link.PublicShareAuthentication, sign bool) (*link.PublicShare, error) {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+
 	db, err := m.persistence.Read(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
-
 	for _, v := range db {
 		passDB := v.(map[string]interface{})["password"].(string)
 		var local link.PublicShare