Skip to content

Commit ca5e2e2

Browse files
d2dyno1d2dyno1
committed
Use thread locks in ChunkAccess
1 parent 9d8ff10 commit ca5e2e2

2 files changed

Lines changed: 172 additions & 109 deletions

File tree

‎src/Core/SecureFolderFS.Core.FileSystem/Chunks/CachingChunkAccess.cs‎

Lines changed: 88 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,18 @@ public override bool FlushAvailable
1818
{
1919
get
2020
{
21+
// Hold the cache lock for the entire operation
2122
lock (_chunkCache)
22-
return _chunkCache.Count > 0;
23+
{
24+
// Only chunks that were actually modified need flushing
25+
foreach (var item in _chunkCache)
26+
{
27+
if (item.Value.WasModified)
28+
return true;
29+
}
30+
31+
return false;
32+
}
2333
}
2434
}
2535

@@ -32,90 +42,109 @@ public CachingChunkAccess(ChunkReader chunkReader, ChunkWriter chunkWriter, ICon
3242
/// <inheritdoc/>
3343
public override int CopyFromChunk(long chunkNumber, Span<byte> destination, int offsetInChunk)
3444
{
35-
// Get chunk
36-
var plaintextChunk = GetChunk(chunkNumber);
37-
if (plaintextChunk is null)
38-
return -1;
45+
// Hold the cache lock for the entire operation
46+
lock (_chunkCache)
47+
{
48+
// Get chunk
49+
var plaintextChunk = GetChunk(chunkNumber);
50+
if (plaintextChunk is null)
51+
return -1;
3952

40-
// Copy from chunk
41-
var count = Math.Min(plaintextChunk.ActualLength - offsetInChunk, destination.Length);
42-
if (count < 0)
43-
return -1;
53+
// Copy from chunk
54+
var count = Math.Min(plaintextChunk.ActualLength - offsetInChunk, destination.Length);
55+
if (count < 0)
56+
return -1;
4457

45-
plaintextChunk.Buffer.AsSpan(offsetInChunk, count).CopyTo(destination);
58+
plaintextChunk.Buffer.AsSpan(offsetInChunk, count).CopyTo(destination);
4659

47-
return count;
60+
return count;
61+
}
4862
}
4963

5064
/// <inheritdoc/>
5165
public override int CopyToChunk(long chunkNumber, ReadOnlySpan<byte> source, int offsetInChunk)
5266
{
53-
// Get chunk
54-
var plaintextChunk = GetChunk(chunkNumber);
55-
if (plaintextChunk is null)
56-
return -1;
67+
// Hold the cache lock for the entire operation
68+
lock (_chunkCache)
69+
{
70+
// Get chunk
71+
var plaintextChunk = GetChunk(chunkNumber);
72+
if (plaintextChunk is null)
73+
return -1;
5774

58-
// Update state of chunk
59-
plaintextChunk.WasModified = true;
75+
// Update state of chunk
76+
plaintextChunk.WasModified = true;
6077

61-
// Copy to chunk
62-
var count = Math.Min(contentCrypt.ChunkPlaintextSize - offsetInChunk, source.Length);
63-
if (count < 0)
64-
return -1;
78+
// Copy to chunk
79+
var count = Math.Min(contentCrypt.ChunkPlaintextSize - offsetInChunk, source.Length);
80+
if (count < 0)
81+
return -1;
6582

66-
var destination = plaintextChunk.Buffer.AsSpan(offsetInChunk, count);
67-
source.Slice(0, count).CopyTo(destination);
83+
var destination = plaintextChunk.Buffer.AsSpan(offsetInChunk, count);
84+
source.Slice(0, count).CopyTo(destination);
6885

69-
// Update actual length
70-
plaintextChunk.ActualLength = Math.Max(plaintextChunk.ActualLength, count + offsetInChunk);
86+
// Update actual length
87+
plaintextChunk.ActualLength = Math.Max(plaintextChunk.ActualLength, count + offsetInChunk);
7188

72-
return count;
89+
return count;
90+
}
7391
}
7492

7593
/// <inheritdoc/>
7694
public override void SetChunkLength(long chunkNumber, int length, bool includeCurrentLength = false)
7795
{
78-
// Get chunk
79-
var plaintextChunk = GetChunk(chunkNumber);
80-
if (plaintextChunk is null)
81-
return;
96+
// Hold the cache lock for the entire operation
97+
lock (_chunkCache)
98+
{
99+
// Get chunk
100+
var plaintextChunk = GetChunk(chunkNumber);
101+
if (plaintextChunk is null)
102+
return;
82103

83-
// Add read length of existing chunk data to the full length if specified
84-
length += includeCurrentLength ? plaintextChunk.ActualLength : 0;
85-
length = Math.Max(length, 0);
104+
// Add read length of existing chunk data to the full length if specified
105+
length += includeCurrentLength ? plaintextChunk.ActualLength : 0;
106+
length = Math.Max(length, 0);
86107

87-
// Determine whether to extend or truncate the chunk
88-
if (length < plaintextChunk.ActualLength)
89-
{
90-
// Truncate chunk
91-
plaintextChunk.ActualLength = Math.Min(plaintextChunk.ActualLength, length);
92-
}
93-
else if (plaintextChunk.ActualLength < length)
94-
{
95-
// Extend chunk
96-
plaintextChunk.ActualLength = Math.Min(length, contentCrypt.ChunkPlaintextSize);
97-
}
98-
else
99-
return; // Ignore resizing the same length
108+
// Determine whether to extend or truncate the chunk
109+
if (length < plaintextChunk.ActualLength)
110+
{
111+
// Truncate chunk
112+
plaintextChunk.ActualLength = Math.Min(plaintextChunk.ActualLength, length);
113+
}
114+
else if (plaintextChunk.ActualLength < length)
115+
{
116+
// Extend chunk
117+
plaintextChunk.ActualLength = Math.Min(length, contentCrypt.ChunkPlaintextSize);
118+
}
119+
else
120+
return; // Ignore resizing the same length
100121

101-
plaintextChunk.WasModified = true;
122+
plaintextChunk.WasModified = true;
123+
}
102124
}
103125

104126
/// <inheritdoc/>
105127
public override void Flush()
106128
{
129+
// Hold the cache lock for the entire operation
107130
lock (_chunkCache)
108131
{
109132
foreach (var item in _chunkCache)
110133
{
111134
if (item.Value.WasModified)
135+
{
112136
chunkWriter.WriteChunk(item.Key, item.Value.Buffer.AsSpan(0, item.Value.ActualLength));
137+
138+
// Mark the chunk as clean so subsequent flushes don't rewrite it
139+
item.Value.WasModified = false;
140+
}
113141
}
114142
}
115143
}
116144

117145
private ChunkBuffer? GetChunk(long chunkNumber)
118146
{
147+
// Hold the cache lock for the entire operation
119148
lock (_chunkCache)
120149
{
121150
if (!_chunkCache.TryGetValue(chunkNumber, out var plaintextChunk))
@@ -147,6 +176,7 @@ public override void Flush()
147176

148177
private void SetChunk(long chunkNumber, ChunkBuffer plaintextChunk)
149178
{
179+
// Hold the cache lock for the entire operation
150180
lock (_chunkCache)
151181
{
152182
if (_chunkCache.Count >= FileSystem.Constants.Caching.RECOMMENDED_SIZE_CHUNKS)
@@ -171,6 +201,17 @@ public override void Dispose()
171201
{
172202
lock (_chunkCache)
173203
{
204+
try
205+
{
206+
// Flush outstanding modified chunks so data is not lost when
207+
// the chunk access is disposed without a prior flush
208+
Flush();
209+
}
210+
catch (Exception)
211+
{
212+
// Dispose must not throw (the backing stream may already be unavailable)
213+
}
214+
174215
base.Dispose();
175216
_chunkCache.Clear();
176217
}

‎src/Core/SecureFolderFS.Core.FileSystem/Chunks/ChunkAccess.cs‎

Lines changed: 84 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,16 @@ internal class ChunkAccess : IDisposable
1616
protected readonly IContentCrypt contentCrypt;
1717
protected readonly IFileSystemStatistics fileSystemStatistics;
1818

19+
/// <summary>
20+
/// The synchronization root guarding chunk operations.
21+
/// </summary>
22+
/// <remarks>
23+
/// A chunk access instance can be shared by multiple streams of the same file,
24+
/// and the reader/writer also share the position of one ciphertext stream,
25+
/// so chunk operations must not interleave.
26+
/// </remarks>
27+
protected readonly object chunkLock = new();
28+
1929
/// <summary>
2030
/// Determines whether there are outstanding chunks ready to be flushed to disk.
2131
/// </summary>
@@ -42,24 +52,28 @@ public virtual int CopyFromChunk(long chunkNumber, Span<byte> destination, int o
4252
var plaintextChunk = ArrayPool<byte>.Shared.Rent(contentCrypt.ChunkPlaintextSize);
4353
try
4454
{
45-
// ArrayPool may return a larger array than requested
46-
var realPlaintextChunk = plaintextChunk.AsSpan(0, contentCrypt.ChunkPlaintextSize);
55+
// Hold the cache lock for the entire operation
56+
lock (chunkLock)
57+
{
58+
// ArrayPool may return a larger array than requested
59+
var realPlaintextChunk = plaintextChunk.AsSpan(0, contentCrypt.ChunkPlaintextSize);
4760

48-
// Read chunk
49-
var read = chunkReader.ReadChunk(chunkNumber, realPlaintextChunk);
61+
// Read chunk
62+
var read = chunkReader.ReadChunk(chunkNumber, realPlaintextChunk);
5063

51-
// Check for any errors
52-
if (read < 0)
53-
return read;
64+
// Check for any errors
65+
if (read < 0)
66+
return read;
5467

55-
// Copy from chunk
56-
var count = Math.Min(read - offsetInChunk, destination.Length);
57-
if (count <= 0)
58-
return 0;
68+
// Copy from chunk
69+
var count = Math.Min(read - offsetInChunk, destination.Length);
70+
if (count <= 0)
71+
return 0;
5972

60-
realPlaintextChunk.Slice(offsetInChunk, count).CopyTo(destination);
73+
realPlaintextChunk.Slice(offsetInChunk, count).CopyTo(destination);
6174

62-
return count;
75+
return count;
76+
}
6377
}
6478
finally
6579
{
@@ -84,28 +98,32 @@ public virtual int CopyToChunk(long chunkNumber, ReadOnlySpan<byte> source, int
8498
var plaintextChunk = ArrayPool<byte>.Shared.Rent(contentCrypt.ChunkPlaintextSize);
8599
try
86100
{
87-
// ArrayPool may return larger array than requested
88-
var realPlaintextChunk = plaintextChunk.AsSpan(0, contentCrypt.ChunkPlaintextSize);
101+
// Hold the cache lock for the entire operation
102+
lock (chunkLock)
103+
{
104+
// ArrayPool may return larger array than requested
105+
var realPlaintextChunk = plaintextChunk.AsSpan(0, contentCrypt.ChunkPlaintextSize);
89106

90-
// Read chunk
91-
var read = chunkReader.ReadChunk(chunkNumber, realPlaintextChunk);
107+
// Read chunk
108+
var read = chunkReader.ReadChunk(chunkNumber, realPlaintextChunk);
92109

93-
// Check for any errors
94-
if (read < 0)
95-
return read;
110+
// Check for any errors
111+
if (read < 0)
112+
return read;
96113

97-
// Copy to chunk
98-
var count = Math.Min(contentCrypt.ChunkPlaintextSize - offsetInChunk, source.Length);
99-
if (count <= 0)
100-
return 0;
114+
// Copy to chunk
115+
var count = Math.Min(contentCrypt.ChunkPlaintextSize - offsetInChunk, source.Length);
116+
if (count <= 0)
117+
return 0;
101118

102-
var destination = realPlaintextChunk.Slice(offsetInChunk, count);
103-
source.Slice(0, count).CopyTo(destination);
119+
var destination = realPlaintextChunk.Slice(offsetInChunk, count);
120+
source.Slice(0, count).CopyTo(destination);
104121

105-
// Write to chunk
106-
chunkWriter.WriteChunk(chunkNumber, destination);
122+
// Write to chunk
123+
chunkWriter.WriteChunk(chunkNumber, destination);
107124

108-
return count;
125+
return count;
126+
}
109127
}
110128
finally
111129
{
@@ -129,41 +147,45 @@ public virtual void SetChunkLength(long chunkNumber, int length, bool includeCur
129147
var plaintextChunk = ArrayPool<byte>.Shared.Rent(contentCrypt.ChunkPlaintextSize);
130148
try
131149
{
132-
// ArrayPool may return larger array than requested
133-
var realPlaintextChunk = plaintextChunk.AsSpan(0, contentCrypt.ChunkPlaintextSize);
134-
135-
// Read chunk
136-
var read = chunkReader.ReadChunk(chunkNumber, realPlaintextChunk);
137-
138-
// Check for any errors
139-
if (read < 0)
140-
throw new CryptographicException();
141-
142-
// Add read length of existing chunk data to the full length if specified
143-
length += includeCurrentLength ? read : 0;
144-
length = Math.Max(length, 0);
145-
146-
Span<byte> newPlaintextChunk;
147-
148-
// Determine whether to extend or truncate the chunk
149-
if (length < read)
150+
// Hold the cache lock for the entire operation
151+
lock (chunkLock)
150152
{
151-
// Truncate chunk
152-
newPlaintextChunk = realPlaintextChunk.Slice(0, Math.Min(read, length));
153+
// ArrayPool may return larger array than requested
154+
var realPlaintextChunk = plaintextChunk.AsSpan(0, contentCrypt.ChunkPlaintextSize);
155+
156+
// Read chunk
157+
var read = chunkReader.ReadChunk(chunkNumber, realPlaintextChunk);
158+
159+
// Check for any errors
160+
if (read < 0)
161+
throw new CryptographicException();
162+
163+
// Add read length of existing chunk data to the full length if specified
164+
length += includeCurrentLength ? read : 0;
165+
length = Math.Max(length, 0);
166+
167+
Span<byte> newPlaintextChunk;
168+
169+
// Determine whether to extend or truncate the chunk
170+
if (length < read)
171+
{
172+
// Truncate chunk
173+
newPlaintextChunk = realPlaintextChunk.Slice(0, Math.Min(read, length));
174+
}
175+
else if (read < length)
176+
{
177+
// Clear residual data from ArrayPool and append zeros
178+
realPlaintextChunk.Slice(read).Clear();
179+
180+
// Extend chunk
181+
newPlaintextChunk = realPlaintextChunk.Slice(0, Math.Min(length, contentCrypt.ChunkPlaintextSize));
182+
}
183+
else
184+
return; // Ignore resizing the same length
185+
186+
// Save newly modified chunk
187+
chunkWriter.WriteChunk(chunkNumber, newPlaintextChunk);
153188
}
154-
else if (read < length)
155-
{
156-
// Clear residual data from ArrayPool and append zeros
157-
realPlaintextChunk.Slice(read).Clear();
158-
159-
// Extend chunk
160-
newPlaintextChunk = realPlaintextChunk.Slice(0, Math.Min(length, contentCrypt.ChunkPlaintextSize));
161-
}
162-
else
163-
return; // Ignore resizing the same length
164-
165-
// Save newly modified chunk
166-
chunkWriter.WriteChunk(chunkNumber, newPlaintextChunk);
167189
}
168190
finally
169191
{

0 commit comments

Comments
 (0)