Skip to content

Commit 0d217ca

Browse files
Fix potential double-close issue in sharedEncryption
Add sync.Once protection to prevent multiple calls to underlying Encryption.Close() when Remove() is called repeatedly. ## Problem - sharedEncryption.Remove() calls s.Encryption.Close() directly - No protection against multiple Remove() calls on same instance - Could cause panic or undefined behavior in underlying encryption ## Solution - Add closeOnce sync.Once field to sharedEncryption struct - Wrap s.Encryption.Close() call in closeOnce.Do() - Ensures Close() called exactly once regardless of Remove() call frequency ## Benefits - **Idempotency**: Remove() can be called safely multiple times - **Race Protection**: Concurrent Remove() calls properly synchronized - **Panic Prevention**: Eliminates double-close panics in underlying encryption - **Zero Cost**: sync.Once has no overhead after first call ## Testing - TestSharedEncryption_DoubleCloseProtection: Verifies multiple Remove() calls - TestSharedEncryption_ConcurrentDoubleClose: Tests concurrent access - TestSharedEncryption_CloseAndRemove: Tests interaction with Close() method 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent 47743bd commit 0d217ca

File tree

2 files changed

+137
-1
lines changed

2 files changed

+137
-1
lines changed

go/appencryption/session_cache.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ type sharedEncryption struct {
2727
accessCounter int
2828
mu *sync.Mutex
2929
cond *sync.Cond
30+
closeOnce sync.Once // Prevents double-close of underlying Encryption
3031
}
3132

3233
func (s *sharedEncryption) incrementUsage() {
@@ -53,7 +54,10 @@ func (s *sharedEncryption) Remove() {
5354
s.cond.Wait()
5455
}
5556

56-
s.Encryption.Close()
57+
// Use sync.Once to prevent double-close
58+
s.closeOnce.Do(func() {
59+
s.Encryption.Close()
60+
})
5761

5862
s.mu.Unlock()
5963
}
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
package appencryption
2+
3+
import (
4+
"context"
5+
"sync"
6+
"testing"
7+
"time"
8+
9+
"github.com/stretchr/testify/assert"
10+
)
11+
12+
// TestSharedEncryption_DoubleCloseProtection verifies that calling Remove() multiple times
13+
// only calls the underlying Encryption.Close() once, preventing panics or undefined behavior.
14+
func TestSharedEncryption_DoubleCloseProtection(t *testing.T) {
15+
var closeCount int
16+
mockEncryption := &testEncryption{
17+
closeFunc: func() error {
18+
closeCount++
19+
return nil
20+
},
21+
}
22+
23+
sharedEnc := &sharedEncryption{
24+
Encryption: mockEncryption,
25+
created: time.Now(),
26+
accessCounter: 0, // No active users
27+
mu: new(sync.Mutex),
28+
}
29+
sharedEnc.cond = sync.NewCond(sharedEnc.mu)
30+
31+
// Call Remove() multiple times
32+
sharedEnc.Remove()
33+
sharedEnc.Remove()
34+
sharedEnc.Remove()
35+
36+
// Verify Close() was called exactly once
37+
assert.Equal(t, 1, closeCount, "Close() should be called exactly once despite multiple Remove() calls")
38+
}
39+
40+
// TestSharedEncryption_ConcurrentDoubleClose tests concurrent calls to Remove()
41+
// to ensure no race conditions in the double-close protection.
42+
func TestSharedEncryption_ConcurrentDoubleClose(t *testing.T) {
43+
var closeCount int
44+
var mu sync.Mutex
45+
mockEncryption := &testEncryption{
46+
closeFunc: func() error {
47+
mu.Lock()
48+
closeCount++
49+
mu.Unlock()
50+
return nil
51+
},
52+
}
53+
54+
sharedEnc := &sharedEncryption{
55+
Encryption: mockEncryption,
56+
created: time.Now(),
57+
accessCounter: 0, // No active users
58+
mu: new(sync.Mutex),
59+
}
60+
sharedEnc.cond = sync.NewCond(sharedEnc.mu)
61+
62+
const numGoroutines = 10
63+
var wg sync.WaitGroup
64+
wg.Add(numGoroutines)
65+
66+
// Launch multiple goroutines calling Remove() concurrently
67+
for i := 0; i < numGoroutines; i++ {
68+
go func() {
69+
defer wg.Done()
70+
sharedEnc.Remove()
71+
}()
72+
}
73+
74+
wg.Wait()
75+
76+
// Verify Close() was called exactly once despite concurrent calls
77+
assert.Equal(t, 1, closeCount, "Close() should be called exactly once despite concurrent Remove() calls")
78+
}
79+
80+
// TestSharedEncryption_CloseAndRemove tests calling both Close() and Remove()
81+
// to ensure they work together correctly.
82+
func TestSharedEncryption_CloseAndRemove(t *testing.T) {
83+
var closeCount int
84+
mockEncryption := &testEncryption{
85+
closeFunc: func() error {
86+
closeCount++
87+
return nil
88+
},
89+
}
90+
91+
sharedEnc := &sharedEncryption{
92+
Encryption: mockEncryption,
93+
created: time.Now(),
94+
accessCounter: 1, // One active user
95+
mu: new(sync.Mutex),
96+
}
97+
sharedEnc.cond = sync.NewCond(sharedEnc.mu)
98+
99+
// Simulate user calling Close() (decrements counter)
100+
err := sharedEnc.Close()
101+
assert.NoError(t, err)
102+
assert.Equal(t, 0, sharedEnc.accessCounter)
103+
104+
// Now call Remove() - should close the underlying encryption
105+
sharedEnc.Remove()
106+
107+
// Call Remove() again - should not double-close
108+
sharedEnc.Remove()
109+
110+
// Verify Close() was called exactly once
111+
assert.Equal(t, 1, closeCount, "Close() should be called exactly once")
112+
}
113+
114+
// testEncryption is a simple test double for the Encryption interface.
115+
type testEncryption struct {
116+
closeFunc func() error
117+
}
118+
119+
func (t *testEncryption) EncryptPayload(ctx context.Context, data []byte) (*DataRowRecord, error) {
120+
return nil, nil
121+
}
122+
123+
func (t *testEncryption) DecryptDataRowRecord(ctx context.Context, record DataRowRecord) ([]byte, error) {
124+
return nil, nil
125+
}
126+
127+
func (t *testEncryption) Close() error {
128+
if t.closeFunc != nil {
129+
return t.closeFunc()
130+
}
131+
return nil
132+
}

0 commit comments

Comments
 (0)