From 4c4dcd4888364b58fa6528036161a5ea684a2291 Mon Sep 17 00:00:00 2001 From: Roman Perekhod Date: Tue, 23 Jan 2024 11:26:41 +0100 Subject: [PATCH 1/2] fixed the race condition that led to concurrent map access in a publicshare manager --- .../fix-concurrent-access-to-map.md | 6 ++++ pkg/publicshare/manager/json/json.go | 34 ++++++++++--------- 2 files changed, 24 insertions(+), 16 deletions(-) create mode 100644 changelog/unreleased/fix-concurrent-access-to-map.md diff --git a/changelog/unreleased/fix-concurrent-access-to-map.md b/changelog/unreleased/fix-concurrent-access-to-map.md new file mode 100644 index 0000000000..4fe1358f53 --- /dev/null +++ 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 index 960a2eea6e..d5a17fd9ee 100644 --- 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 From bfb4df7c54e03f6c43df98712d143a7e4dee8ec3 Mon Sep 17 00:00:00 2001 From: Roman Perekhod Date: Tue, 23 Jan 2024 12:28:18 +0100 Subject: [PATCH 2/2] clear up --- pkg/publicshare/manager/json/json.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go index d5a17fd9ee..7e809f5528 100644 --- a/pkg/publicshare/manager/json/json.go +++ b/pkg/publicshare/manager/json/json.go @@ -450,7 +450,7 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu if ref.GetId().GetOpaqueId() == ps.Id.OpaqueId { if publicshare.IsExpired(ps) { - if err := m.revokeExpiredPublicShare(ctx, &ps, u); err != nil { + if err := m.revokeExpiredPublicShare(ctx, &ps); err != nil { return nil, err } return nil, errtypes.NotFound("no shares found by id:" + ref.GetId().String()) @@ -494,7 +494,7 @@ func (m *manager) ListPublicShares(ctx context.Context, u *user.User, filters [] } if publicshare.IsExpired(local.PublicShare) { - if err := m.revokeExpiredPublicShare(ctx, &local.PublicShare, u); err != nil { + if err := m.revokeExpiredPublicShare(ctx, &local.PublicShare); err != nil { log.Error().Err(err). Str("share_token", local.Token). Msg("failed to revoke expired public share") @@ -564,18 +564,18 @@ func (m *manager) cleanupExpiredShares() { _ = utils.UnmarshalJSONToProtoV1([]byte(d.(string)), &ps) if publicshare.IsExpired(ps) { - _ = m.revokeExpiredPublicShare(context.Background(), &ps, nil) + _ = m.revokeExpiredPublicShare(context.Background(), &ps) } } } // 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 { +func (m *manager) revokeExpiredPublicShare(ctx context.Context, s *link.PublicShare) error { if !m.enableExpiredSharesCleanup { return nil } - err := m.revokePublicShare(ctx, u, &link.PublicShareReference{ + err := m.revokePublicShare(ctx, &link.PublicShareReference{ Spec: &link.PublicShareReference_Id{ Id: &link.PublicShareId{ OpaqueId: s.Id.OpaqueId, @@ -591,14 +591,14 @@ 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 { +func (m *manager) RevokePublicShare(ctx context.Context, _ *user.User, ref *link.PublicShareReference) error { m.mutex.Lock() defer m.mutex.Unlock() - return m.revokePublicShare(ctx, u, ref) + return m.revokePublicShare(ctx, 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 { +func (m *manager) revokePublicShare(ctx context.Context, ref *link.PublicShareReference) error { db, err := m.persistence.Read(ctx) if err != nil { return err @@ -665,8 +665,7 @@ func (m *manager) GetPublicShareByToken(ctx context.Context, token string, auth if local.Token == token { if publicshare.IsExpired(local) { - // TODO user is not needed at all in this API. - if err := m.revokeExpiredPublicShare(ctx, &local, nil); err != nil { + if err := m.revokeExpiredPublicShare(ctx, &local); err != nil { return nil, err } break