Skip to content

Commit 506b9a1

Browse files
alinpahontu2912alinpahontu2912
authored
Fix ZipArchive Update mode rewriting unchanged archives on Dispose (dotnet#123719)
Fix ZipArchive in Update mode rewriting when no changes made Opening an entry stream in ZipArchiveMode.Update would immediately mark the archive as modified, causing Dispose to attempt a rewrite even when no actual writes occurred. This threw NotSupportedException on non-expandable streams like fixed-size MemoryStream. The fix tracks actual writes via WrappedStream and only marks the entry as modified when data is written. Archives created on empty streams are marked as new to ensure they're still written on first creation. Fixes dotnet#123419
1 parent b7d32ac commit 506b9a1

File tree

8 files changed

+303
-49
lines changed

8 files changed

+303
-49
lines changed

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.Async.cs‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,10 +148,24 @@ protected virtual async ValueTask DisposeAsyncCore()
148148
case ZipArchiveMode.Read:
149149
break;
150150
case ZipArchiveMode.Create:
151+
await WriteFileAsync().ConfigureAwait(false);
152+
break;
151153
case ZipArchiveMode.Update:
152154
default:
153-
Debug.Assert(_mode == ZipArchiveMode.Update || _mode == ZipArchiveMode.Create);
154-
await WriteFileAsync().ConfigureAwait(false);
155+
Debug.Assert(_mode == ZipArchiveMode.Update);
156+
// Only write if the archive has been modified
157+
if (IsModified)
158+
{
159+
await WriteFileAsync().ConfigureAwait(false);
160+
}
161+
else
162+
{
163+
// Even if we didn't write, unload any entry buffers that may have been loaded
164+
foreach (ZipArchiveEntry entry in _entries)
165+
{
166+
await entry.UnloadStreamsAsync().ConfigureAwait(false);
167+
}
168+
}
155169
break;
156170
}
157171
}

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs‎

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -300,10 +300,24 @@ protected virtual void Dispose(bool disposing)
300300
case ZipArchiveMode.Read:
301301
break;
302302
case ZipArchiveMode.Create:
303+
WriteFile();
304+
break;
303305
case ZipArchiveMode.Update:
304306
default:
305-
Debug.Assert(_mode == ZipArchiveMode.Update || _mode == ZipArchiveMode.Create);
306-
WriteFile();
307+
Debug.Assert(_mode == ZipArchiveMode.Update);
308+
// Only write if the archive has been modified
309+
if (IsModified)
310+
{
311+
WriteFile();
312+
}
313+
else
314+
{
315+
// Even if we didn't write, unload any entry buffers that may have been loaded
316+
foreach (ZipArchiveEntry entry in _entries)
317+
{
318+
entry.UnloadStreams();
319+
}
320+
}
307321
break;
308322
}
309323
}
@@ -314,7 +328,6 @@ protected virtual void Dispose(bool disposing)
314328
}
315329
}
316330
}
317-
318331
/// <summary>
319332
/// Finishes writing the archive and releases all resources used by the ZipArchive object, unless the object was constructed with leaveOpen as true. Any streams from opened entries in the ZipArchive still open will throw exceptions on subsequent writes, as the underlying streams will have been closed.
320333
/// </summary>
@@ -382,6 +395,39 @@ private set
382395
// New entries in the archive won't change its state.
383396
internal ChangeState Changed { get; private set; }
384397

398+
/// <summary>
399+
/// Determines whether the archive has been modified and needs to be written.
400+
/// </summary>
401+
private bool IsModified
402+
{
403+
get
404+
{
405+
// A new archive (created on empty stream) always needs to write the structure
406+
if (_archiveStream.Length == 0)
407+
{
408+
return true;
409+
}
410+
// Archive-level changes (e.g., comment)
411+
if (Changed != ChangeState.Unchanged)
412+
{
413+
return true;
414+
}
415+
// Any deleted entries
416+
if (_firstDeletedEntryOffset != long.MaxValue)
417+
{
418+
return true;
419+
}
420+
// Check if any entry was modified or added
421+
foreach (ZipArchiveEntry entry in _entries)
422+
{
423+
if (!entry.OriginallyInArchive || entry.Changes != ChangeState.Unchanged)
424+
return true;
425+
}
426+
427+
return false;
428+
}
429+
}
430+
385431
private ZipArchiveEntry DoCreateEntry(string entryName, CompressionLevel? compressionLevel)
386432
{
387433
ArgumentException.ThrowIfNullOrEmpty(entryName);

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -143,12 +143,6 @@ private async Task<MemoryStream> GetUncompressedDataAsync(CancellationToken canc
143143
}
144144
}
145145
}
146-
147-
// if they start modifying it and the compression method is not "store", we should make sure it will get deflated
148-
if (CompressionMethod != ZipCompressionMethod.Stored)
149-
{
150-
CompressionMethod = ZipCompressionMethod.Deflate;
151-
}
152146
}
153147

154148
return _storedUncompressedData;
@@ -270,8 +264,6 @@ private async Task<WrappedStream> OpenInUpdateModeAsync(bool loadExistingContent
270264
await ThrowIfNotOpenableAsync(needToUncompress: true, needToLoadIntoMemory: true, cancellationToken).ConfigureAwait(false);
271265
}
272266

273-
_everOpenedForWrite = true;
274-
Changes |= ZipArchive.ChangeState.StoredData;
275267
_currentlyOpenForWrite = true;
276268

277269
if (loadExistingContent)
@@ -282,14 +274,15 @@ private async Task<WrappedStream> OpenInUpdateModeAsync(bool loadExistingContent
282274
{
283275
_storedUncompressedData?.Dispose();
284276
_storedUncompressedData = new MemoryStream();
277+
// Opening with loadExistingContent: false discards existing content, which is a modification
278+
MarkAsModified();
285279
}
286280

287281
_storedUncompressedData.Seek(0, SeekOrigin.Begin);
288282

289-
return new WrappedStream(_storedUncompressedData, this, thisRef =>
290-
{
291-
thisRef!._currentlyOpenForWrite = false;
292-
});
283+
return new WrappedStream(_storedUncompressedData, this,
284+
onClosed: thisRef => thisRef!._currentlyOpenForWrite = false,
285+
notifyEntryOnWrite: true);
293286
}
294287

295288
private async Task<(bool, string?)> IsOpenableAsync(bool needToUncompress, bool needToLoadIntoMemory, CancellationToken cancellationToken)
@@ -351,6 +344,19 @@ private async Task<bool> WriteLocalFileHeaderAsync(bool isEmptyFile, bool forceW
351344
private async Task WriteLocalFileHeaderAndDataIfNeededAsync(bool forceWrite, CancellationToken cancellationToken)
352345
{
353346
cancellationToken.ThrowIfCancellationRequested();
347+
348+
// Check if the entry's stored data was actually modified (StoredData flag is set).
349+
// If _storedUncompressedData is loaded but StoredData is not set, it means the entry
350+
// was opened for update but no writes occurred - we should use the original compressed bytes.
351+
bool storedDataModified = (Changes & ZipArchive.ChangeState.StoredData) != 0;
352+
353+
// If _storedUncompressedData is loaded but not modified, clear it so we use _compressedBytes
354+
if (_storedUncompressedData != null && !storedDataModified)
355+
{
356+
await _storedUncompressedData.DisposeAsync().ConfigureAwait(false);
357+
_storedUncompressedData = null;
358+
}
359+
354360
// _storedUncompressedData gets frozen here, and is what gets written to the file
355361
if (_storedUncompressedData != null || _compressedBytes != null)
356362
{
@@ -466,7 +472,7 @@ private ValueTask WriteDataDescriptorAsync(CancellationToken cancellationToken)
466472
return _archive.ArchiveStream.WriteAsync(dataDescriptor.AsMemory(0, bytesToWrite), cancellationToken);
467473
}
468474

469-
private async Task UnloadStreamsAsync()
475+
internal async Task UnloadStreamsAsync()
470476
{
471477
if (_storedUncompressedData != null)
472478
{

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs‎

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -508,16 +508,15 @@ private MemoryStream GetUncompressedData()
508508
}
509509
}
510510

511-
// if they start modifying it and the compression method is not "store", we should make sure it will get deflated
512-
if (CompressionMethod != ZipCompressionMethod.Stored)
513-
{
514-
CompressionMethod = ZipCompressionMethod.Deflate;
515-
}
511+
// NOTE: CompressionMethod normalization is deferred to MarkAsModified() to avoid
512+
// corrupting entries that are opened in Update mode but not actually written to.
513+
// If we normalized here and the entry wasn't modified, we'd write a header with
514+
// CompressionMethod=Deflate but the original _compressedBytes would still be in
515+
// their original format (e.g., Deflate64), producing an invalid entry.
516516
}
517517

518518
return _storedUncompressedData;
519519
}
520-
521520
// does almost everything you need to do to forget about this entry
522521
// writes the local header/data, gets rid of all the data,
523522
// closes all of the streams except for the very outermost one that
@@ -882,8 +881,6 @@ private WrappedStream OpenInUpdateMode(bool loadExistingContent = true)
882881
ThrowIfNotOpenable(needToUncompress: true, needToLoadIntoMemory: true);
883882
}
884883

885-
_everOpenedForWrite = true;
886-
Changes |= ZipArchive.ChangeState.StoredData;
887884
_currentlyOpenForWrite = true;
888885

889886
if (loadExistingContent)
@@ -894,14 +891,31 @@ private WrappedStream OpenInUpdateMode(bool loadExistingContent = true)
894891
{
895892
_storedUncompressedData?.Dispose();
896893
_storedUncompressedData = new MemoryStream();
894+
// Opening with loadExistingContent: false discards existing content, which is a modification
895+
MarkAsModified();
897896
}
898897

899898
_storedUncompressedData.Seek(0, SeekOrigin.Begin);
900899

901-
return new WrappedStream(_storedUncompressedData, this, thisRef =>
900+
return new WrappedStream(_storedUncompressedData, this,
901+
onClosed: thisRef => thisRef!._currentlyOpenForWrite = false,
902+
notifyEntryOnWrite: true);
903+
}
904+
905+
/// <summary>
906+
/// Marks this entry as modified, indicating that its data has changed and needs to be rewritten.
907+
/// </summary>
908+
internal void MarkAsModified()
909+
{
910+
_everOpenedForWrite = true;
911+
Changes |= ZipArchive.ChangeState.StoredData;
912+
913+
// Normalize compression method when actually modifying - Deflate64 data will be
914+
// re-compressed as Deflate since we don't support writing Deflate64.
915+
if (CompressionMethod != ZipCompressionMethod.Stored)
902916
{
903-
thisRef!._currentlyOpenForWrite = false;
904-
});
917+
CompressionMethod = ZipCompressionMethod.Deflate;
918+
}
905919
}
906920

907921
private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out string? message)
@@ -1171,6 +1185,18 @@ private bool WriteLocalFileHeader(bool isEmptyFile, bool forceWrite)
11711185

11721186
private void WriteLocalFileHeaderAndDataIfNeeded(bool forceWrite)
11731187
{
1188+
// Check if the entry's stored data was actually modified (StoredData flag is set).
1189+
// If _storedUncompressedData is loaded but StoredData is not set, it means the entry
1190+
// was opened for update but no writes occurred - we should use the original compressed bytes.
1191+
bool storedDataModified = (Changes & ZipArchive.ChangeState.StoredData) != 0;
1192+
1193+
// If _storedUncompressedData is loaded but not modified, clear it so we use _compressedBytes
1194+
if (_storedUncompressedData != null && !storedDataModified)
1195+
{
1196+
_storedUncompressedData.Dispose();
1197+
_storedUncompressedData = null;
1198+
}
1199+
11741200
// _storedUncompressedData gets frozen here, and is what gets written to the file
11751201
if (_storedUncompressedData != null || _compressedBytes != null)
11761202
{
@@ -1396,7 +1422,7 @@ private int PrepareToWriteDataDescriptor(Span<byte> dataDescriptor)
13961422
return bytesToWrite;
13971423
}
13981424

1399-
private void UnloadStreams()
1425+
internal void UnloadStreams()
14001426
{
14011427
_storedUncompressedData?.Dispose();
14021428
_compressedBytes = null;

‎src/libraries/System.IO.Compression/src/System/IO/Compression/ZipCustomStreams.cs‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,24 +16,28 @@ internal sealed class WrappedStream : Stream
1616
// Delegate that will be invoked on stream disposing
1717
private readonly Action<ZipArchiveEntry?>? _onClosed;
1818

19+
// When true, notifies the entry on first write operation
20+
private bool _notifyEntryOnWrite;
21+
1922
// Instance that will be passed to _onClose delegate
2023
private readonly ZipArchiveEntry? _zipArchiveEntry;
2124
private bool _isDisposed;
2225

2326
internal WrappedStream(Stream baseStream, bool closeBaseStream)
24-
: this(baseStream, closeBaseStream, null, null) { }
27+
: this(baseStream, closeBaseStream, entry: null, onClosed: null, notifyEntryOnWrite: false) { }
2528

26-
private WrappedStream(Stream baseStream, bool closeBaseStream, ZipArchiveEntry? entry, Action<ZipArchiveEntry?>? onClosed)
29+
private WrappedStream(Stream baseStream, bool closeBaseStream, ZipArchiveEntry? entry, Action<ZipArchiveEntry?>? onClosed, bool notifyEntryOnWrite)
2730
{
2831
_baseStream = baseStream;
2932
_closeBaseStream = closeBaseStream;
3033
_onClosed = onClosed;
34+
_notifyEntryOnWrite = notifyEntryOnWrite;
3135
_zipArchiveEntry = entry;
3236
_isDisposed = false;
3337
}
3438

35-
internal WrappedStream(Stream baseStream, ZipArchiveEntry entry, Action<ZipArchiveEntry?>? onClosed)
36-
: this(baseStream, false, entry, onClosed) { }
39+
internal WrappedStream(Stream baseStream, ZipArchiveEntry entry, Action<ZipArchiveEntry?>? onClosed, bool notifyEntryOnWrite = false)
40+
: this(baseStream, false, entry, onClosed, notifyEntryOnWrite) { }
3741

3842
public override long Length
3943
{
@@ -144,6 +148,7 @@ public override void SetLength(long value)
144148
ThrowIfCantSeek();
145149
ThrowIfCantWrite();
146150

151+
NotifyWrite();
147152
_baseStream.SetLength(value);
148153
}
149154

@@ -152,6 +157,7 @@ public override void Write(byte[] buffer, int offset, int count)
152157
ThrowIfDisposed();
153158
ThrowIfCantWrite();
154159

160+
NotifyWrite();
155161
_baseStream.Write(buffer, offset, count);
156162
}
157163

@@ -160,6 +166,7 @@ public override void Write(ReadOnlySpan<byte> source)
160166
ThrowIfDisposed();
161167
ThrowIfCantWrite();
162168

169+
NotifyWrite();
163170
_baseStream.Write(source);
164171
}
165172

@@ -168,6 +175,7 @@ public override void WriteByte(byte value)
168175
ThrowIfDisposed();
169176
ThrowIfCantWrite();
170177

178+
NotifyWrite();
171179
_baseStream.WriteByte(value);
172180
}
173181

@@ -176,6 +184,7 @@ public override Task WriteAsync(byte[] buffer, int offset, int count, Cancellati
176184
ThrowIfDisposed();
177185
ThrowIfCantWrite();
178186

187+
NotifyWrite();
179188
return _baseStream.WriteAsync(buffer, offset, count, cancellationToken);
180189
}
181190

@@ -184,9 +193,19 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo
184193
ThrowIfDisposed();
185194
ThrowIfCantWrite();
186195

196+
NotifyWrite();
187197
return _baseStream.WriteAsync(buffer, cancellationToken);
188198
}
189199

200+
private void NotifyWrite()
201+
{
202+
if (_notifyEntryOnWrite)
203+
{
204+
_zipArchiveEntry?.MarkAsModified();
205+
_notifyEntryOnWrite = false; // Only notify once
206+
}
207+
}
208+
190209
public override void Flush()
191210
{
192211
ThrowIfDisposed();

0 commit comments

Comments
 (0)