git.iko.soy/crush
on main help sign in
fix: potential data race on `permissionService` (#2964)
6b312bemei2jun1
internal/permission/permission.go+8−6
compare with main
unifiedsplit
@@ -6,6 +6,7 @@ import (
6 "path/filepath"
6 "path/filepath"
7 "slices"
7 "slices"
8 "sync"
8 "sync"
9+ "sync/atomic"
9
10
10 "github.com/charmbracelet/crush/internal/csync"
11 "github.com/charmbracelet/crush/internal/csync"
11 "github.com/charmbracelet/crush/internal/pubsub"
12 "github.com/charmbracelet/crush/internal/pubsub"
@@ -90,7 +91,7 @@ type permissionService struct {
90 pendingRequests *csync.Map[string, chan bool]
91 pendingRequests *csync.Map[string, chan bool]
91 autoApproveSessions map[string]bool
92 autoApproveSessions map[string]bool
92 autoApproveSessionsMu sync.RWMutex
93 autoApproveSessionsMu sync.RWMutex
93− skip bool
94+ skip atomic.Bool
94 allowedTools []string
95 allowedTools []string
95
96
96 // used to make sure we only process one request at a time
97 // used to make sure we only process one request at a time
@@ -159,7 +160,7 @@ func (s *permissionService) Deny(permission PermissionRequest) {
159}
160}
160
161
161func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) {
162func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) {
162− if s.skip {
163+ if s.skip.Load() {
163 return true, nil
164 return true, nil
164 }
165 }
165
166
@@ -268,22 +269,23 @@ func (s *permissionService) SubscribeNotifications(ctx context.Context) <-chan p
268}
269}
269
270
270func (s *permissionService) SetSkipRequests(skip bool) {
271func (s *permissionService) SetSkipRequests(skip bool) {
271− s.skip = skip
272+ s.skip.Store(skip)
272}
273}
273
274
274func (s *permissionService) SkipRequests() bool {
275func (s *permissionService) SkipRequests() bool {
275− return s.skip
276+ return s.skip.Load()
276}
277}
277
278
278func NewPermissionService(workingDir string, skip bool, allowedTools []string) Service {
279func NewPermissionService(workingDir string, skip bool, allowedTools []string) Service {
279− return &permissionService{
280+ svc := &permissionService{
280 Broker: pubsub.NewBroker[PermissionRequest](),
281 Broker: pubsub.NewBroker[PermissionRequest](),
281 notificationBroker: pubsub.NewBroker[PermissionNotification](),
282 notificationBroker: pubsub.NewBroker[PermissionNotification](),
282 workingDir: workingDir,
283 workingDir: workingDir,
283 sessionPermissions: csync.NewMap[PermissionKey, bool](),
284 sessionPermissions: csync.NewMap[PermissionKey, bool](),
284 autoApproveSessions: make(map[string]bool),
285 autoApproveSessions: make(map[string]bool),
285− skip: skip,
286 allowedTools: allowedTools,
286 allowedTools: allowedTools,
287 pendingRequests: csync.NewMap[string, chan bool](),
287 pendingRequests: csync.NewMap[string, chan bool](),
288 }
288 }
289+ svc.skip.Store(skip)
290+ return svc
289}
291}
internal/permission/permission_test.go+15
@@ -79,6 +79,21 @@ func TestPermissionService_AllowedCommands(t *testing.T) {
79 }
79 }
80}
80}
81
81
82+func TestSkipRace(t *testing.T) {
83+ svc := NewPermissionService("/tmp", false, nil)
84+ var wg sync.WaitGroup
85+ wg.Add(2)
86+ go func() {
87+ defer wg.Done()
88+ svc.SetSkipRequests(true)
89+ }()
90+ go func() {
91+ defer wg.Done()
92+ svc.SkipRequests()
93+ }()
94+ wg.Wait()
95+}
96+
82func TestPermissionService_SkipMode(t *testing.T) {
97func TestPermissionService_SkipMode(t *testing.T) {
83 service := NewPermissionService("/tmp", true, []string{})
98 service := NewPermissionService("/tmp", true, []string{})
84
99