Preserve trailing extra data when updating ZIP files (#113306)

edwardneal · carlossanlop · web-flow · commit f93aa8a3d743 · 2025-04-14T19:11:26.000-06:00
* Reimplement PR 112032
* Permit trailing malformed extra field data. This data is generated by zipalign. It needs to be accounted for and preserved.
* Update test, adding test cases
Prove functionality where there are 3, 4, 7, 8 and 15 bytes of padding. This covers zero, one and multiple well-formed extra fields, with and without subsequent trailing data.
---------
Co-authored-by: Carlos Sánchez López &lt;1175054+carlossanlop@users.noreply.github.com&gt;
diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
@@ -733,13 +733,17 @@ private void WriteFile()
                         if (entry.OffsetOfLocalHeader >= startingOffset)
                         {
                             // If the pending data to write is fixed-length metadata in the header, there's no need to load the compressed file bits.
+                            // We always need to load the local file header's metadata though - at this point, this entry will be written out and we
+                            // want to make sure that we preserve that metadata.
                             if ((entry.Changes & (ChangeState.DynamicLengthMetadata | ChangeState.StoredData)) != 0)
                             {
                                 completeRewriteStartingOffset = Math.Min(completeRewriteStartingOffset, entry.OffsetOfLocalHeader);
                             }
+
+                            entry.LoadLocalHeaderExtraFieldIfNeeded();
                             if (entry.OffsetOfLocalHeader >= completeRewriteStartingOffset)
                             {
-                                entry.LoadLocalHeaderExtraFieldAndCompressedBytesIfNeeded();
+                                entry.LoadCompressedBytesIfNeeded();
                             }
 
                             entriesToWrite.Add(entry);
diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
@@ -43,7 +43,9 @@ public partial class ZipArchiveEntry
         private byte[] _storedEntryNameBytes;
         // only apply to update mode
         private List<ZipGenericExtraField>? _cdUnknownExtraFields;
+        private byte[]? _cdTrailingExtraFieldData;
         private List<ZipGenericExtraField>? _lhUnknownExtraFields;
+        private byte[]? _lhTrailingExtraFieldData;
         private byte[] _fileComment;
         private readonly CompressionLevel _compressionLevel;
 
@@ -53,6 +55,11 @@ internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd)
             _archive = archive;
 
             _originallyInArchive = true;
+            // It's possible for the CompressionMethod setter and DetectEntryNameVersion to update this, even without any explicit
+            // changes. This can occur if a ZipArchive instance runs in Update mode and opens a stream with invalid data. In such
+            // a situation, both the local file header and the central directory header will be rewritten (to prevent the headers
+            // from falling out of sync when the central directory header is rewritten.)
+            Changes = ZipArchive.ChangeState.Unchanged;
 
             _diskNumberStart = cd.DiskNumberStart;
             _versionMadeByPlatform = (ZipVersionMadeByPlatform)cd.VersionMadeByCompatibility;
@@ -84,12 +91,11 @@ internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd)
             _lhUnknownExtraFields = null;
             // the cd should have this as null if we aren't in Update mode
             _cdUnknownExtraFields = cd.ExtraFields;
+            _cdTrailingExtraFieldData = cd.TrailingExtraFieldData;
 
             _fileComment = cd.FileComment;
 
             _compressionLevel = MapCompressionLevel(_generalPurposeBitFlag, CompressionMethod);
-
-            Changes = ZipArchive.ChangeState.Unchanged;
         }
 
         // Initializes a ZipArchiveEntry instance for a new archive entry with a specified compression level.
@@ -543,8 +549,9 @@ internal void WriteCentralDirectoryFileHeader(bool forceWrite)
 
 
             // determine if we can fit zip64 extra field and original extra fields all in
+            int currExtraFieldDataLength = ZipGenericExtraField.TotalSize(_cdUnknownExtraFields, _cdTrailingExtraFieldData?.Length ?? 0);
             int bigExtraFieldLength = (zip64ExtraField != null ? zip64ExtraField.TotalSize : 0)
-                                      + (_cdUnknownExtraFields != null ? ZipGenericExtraField.TotalSize(_cdUnknownExtraFields) : 0);
+                                      + currExtraFieldDataLength;
             ushort extraFieldLength;
             if (bigExtraFieldLength > ushort.MaxValue)
             {
@@ -561,7 +568,7 @@ internal void WriteCentralDirectoryFileHeader(bool forceWrite)
                 long centralDirectoryHeaderLength = ZipCentralDirectoryFileHeader.FieldLocations.DynamicData
                     + _storedEntryNameBytes.Length
                     + (zip64ExtraField != null ? zip64ExtraField.TotalSize : 0)
-                    + (_cdUnknownExtraFields != null ? ZipGenericExtraField.TotalSize(_cdUnknownExtraFields) : 0)
+                    + currExtraFieldDataLength
                     + _fileComment.Length;
 
                 _archive.ArchiveStream.Seek(centralDirectoryHeaderLength, SeekOrigin.Current);
@@ -609,13 +616,11 @@ internal void WriteCentralDirectoryFileHeader(bool forceWrite)
                 _archive.ArchiveStream.Write(cdStaticHeader);
                 _archive.ArchiveStream.Write(_storedEntryNameBytes);
 
-                // write extra fields, and only write zip64ExtraField if we decided we need it (it's not null)
+                // only write zip64ExtraField if we decided we need it (it's not null)
                 zip64ExtraField?.WriteBlock(_archive.ArchiveStream);
 
-                if (_cdUnknownExtraFields != null)
-                {
-                    ZipGenericExtraField.WriteAllBlocks(_cdUnknownExtraFields, _archive.ArchiveStream);
-                }
+                // write extra fields (and any malformed trailing data).
+                ZipGenericExtraField.WriteAllBlocks(_cdUnknownExtraFields, _cdTrailingExtraFieldData ?? Array.Empty<byte>(), _archive.ArchiveStream);
 
                 if (_fileComment.Length > 0)
                 {
@@ -626,7 +631,7 @@ internal void WriteCentralDirectoryFileHeader(bool forceWrite)
 
         // throws exception if fails, will get called on every relevant entry before closing in update mode
         // can throw InvalidDataException
-        internal void LoadLocalHeaderExtraFieldAndCompressedBytesIfNeeded()
+        internal void LoadLocalHeaderExtraFieldIfNeeded()
         {
             // we should have made this exact call in _archive.Init through ThrowIfOpenable
             Debug.Assert(IsOpenable(false, true, out _));
@@ -635,8 +640,16 @@ internal void LoadLocalHeaderExtraFieldAndCompressedBytesIfNeeded()
             if (_originallyInArchive)
             {
                 _archive.ArchiveStream.Seek(_offsetOfLocalHeader, SeekOrigin.Begin);
-                _lhUnknownExtraFields = ZipLocalFileHeader.GetExtraFields(_archive.ArchiveStream);
+                _lhUnknownExtraFields = ZipLocalFileHeader.GetExtraFields(_archive.ArchiveStream, out _lhTrailingExtraFieldData);
             }
+        }
+
+        // throws exception if fails, will get called on every relevant entry before closing in update mode
+        // can throw InvalidDataException
+        internal void LoadCompressedBytesIfNeeded()
+        {
+            // we should have made this exact call in _archive.Init through ThrowIfOpenable
+            Debug.Assert(IsOpenable(false, true, out _));
 
             if (!_everOpenedForWrite && _originallyInArchive)
             {
@@ -979,8 +992,9 @@ private bool WriteLocalFileHeader(bool isEmptyFile, bool forceWrite)
             _offsetOfLocalHeader = _archive.ArchiveStream.Position;
 
             // calculate extra field. if zip64 stuff + original extraField aren't going to fit, dump the original extraField, because this is more important
+            int currExtraFieldDataLength = ZipGenericExtraField.TotalSize(_lhUnknownExtraFields, _lhTrailingExtraFieldData?.Length ?? 0);
             int bigExtraFieldLength = (zip64ExtraField != null ? zip64ExtraField.TotalSize : 0)
-                                      + (_lhUnknownExtraFields != null ? ZipGenericExtraField.TotalSize(_lhUnknownExtraFields) : 0);
+                                      + currExtraFieldDataLength;
             ushort extraFieldLength;
             if (bigExtraFieldLength > ushort.MaxValue)
             {
@@ -1003,10 +1017,7 @@ private bool WriteLocalFileHeader(bool isEmptyFile, bool forceWrite)
                     _archive.ArchiveStream.Seek(zip64ExtraField.TotalSize, SeekOrigin.Current);
                 }
 
-                if (_lhUnknownExtraFields != null)
-                {
-                    _archive.ArchiveStream.Seek(ZipGenericExtraField.TotalSize(_lhUnknownExtraFields), SeekOrigin.Current);
-                }
+                _archive.ArchiveStream.Seek(currExtraFieldDataLength, SeekOrigin.Current);
             }
             else
             {
@@ -1029,8 +1040,7 @@ private bool WriteLocalFileHeader(bool isEmptyFile, bool forceWrite)
                 // Only when handling zip64
                 zip64ExtraField?.WriteBlock(_archive.ArchiveStream);
 
-                if (_lhUnknownExtraFields != null)
-                    ZipGenericExtraField.WriteAllBlocks(_lhUnknownExtraFields, _archive.ArchiveStream);
+                ZipGenericExtraField.WriteAllBlocks(_lhUnknownExtraFields, _lhTrailingExtraFieldData ?? Array.Empty<byte>(), _archive.ArchiveStream);
             }
 
             return zip64ExtraField != null;
@@ -1252,10 +1262,12 @@ private void VersionToExtractAtLeast(ZipVersionNeededValues value)
             if (_versionToExtract < value)
             {
                 _versionToExtract = value;
+                Changes |= ZipArchive.ChangeState.FixedLengthMetadata;
             }
             if (_versionMadeBySpecification < value)
             {
                 _versionMadeBySpecification = value;
+                Changes |= ZipArchive.ChangeState.FixedLengthMetadata;
             }
         }
 
diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
@@ -48,7 +48,6 @@ public static bool TryReadBlock(ReadOnlySpan<byte> bytes, out int bytesConsumed,
 
             field._tag = BinaryPrimitives.ReadUInt16LittleEndian(bytes[FieldLocations.Tag..]);
             field._size = BinaryPrimitives.ReadUInt16LittleEndian(bytes[FieldLocations.Size..]);
-            bytesConsumed += SizeOfHeader;
 
             // not enough byte to read the data
             if ((bytes.Length - SizeOfHeader) < field._size)
@@ -57,11 +56,11 @@ public static bool TryReadBlock(ReadOnlySpan<byte> bytes, out int bytesConsumed,
             }
 
             field._data = bytes.Slice(FieldLocations.DynamicData, field._size).ToArray();
-            bytesConsumed += field._size;
+            bytesConsumed = field.Size + SizeOfHeader;
             return true;
         }
 
-        public static List<ZipGenericExtraField> ParseExtraField(ReadOnlySpan<byte> extraFieldData)
+        public static List<ZipGenericExtraField> ParseExtraField(ReadOnlySpan<byte> extraFieldData, out ReadOnlySpan<byte> trailingExtraFieldData)
         {
             List<ZipGenericExtraField> extraFields = new List<ZipGenericExtraField>();
             int totalBytesConsumed = 0;
@@ -71,25 +70,43 @@ public static List<ZipGenericExtraField> ParseExtraField(ReadOnlySpan<byte> extr
                 totalBytesConsumed += currBytesConsumed;
                 extraFields.Add(field);
             }
+            // It's possible for some ZIP files to contain extra data which isn't a well-formed extra field. zipalign does this, padding the extra data
+            // field with zeroes to align the start of the file data to an X-byte boundary. We need to preserve this data so that the recorded "extra data length"
+            // fields in the central directory and local file headers are valid.
+            // We need to account for this because zipalign is part of the build process for Android applications, and failing to do this will result in
+            // .NET for Android generating corrupted .apk files and failing to build.
+            trailingExtraFieldData = extraFieldData[totalBytesConsumed..];
 
             return extraFields;
         }
 
-        public static int TotalSize(List<ZipGenericExtraField> fields)
+        public static int TotalSize(List<ZipGenericExtraField>? fields, int trailingDataLength)
         {
-            int size = 0;
-            foreach (ZipGenericExtraField field in fields)
+            int size = trailingDataLength;
+
+            if (fields != null)
             {
-                size += field.Size + SizeOfHeader; //size is only size of data
+                foreach (ZipGenericExtraField field in fields)
+                {
+                    size += field.Size + SizeOfHeader; //size is only size of data
+                }
             }
             return size;
         }
 
-        public static void WriteAllBlocks(List<ZipGenericExtraField> fields, Stream stream)
+        public static void WriteAllBlocks(List<ZipGenericExtraField>? fields, ReadOnlySpan<byte> trailingExtraFieldData, Stream stream)
         {
-            foreach (ZipGenericExtraField field in fields)
+            if (fields != null)
+            {
+                foreach (ZipGenericExtraField field in fields)
+                {
+                    field.WriteBlock(stream);
+                }
+            }
+
+            if (!trailingExtraFieldData.IsEmpty)
             {
-                field.WriteBlock(stream);
+                stream.Write(trailingExtraFieldData);
             }
         }
     }
@@ -514,7 +531,7 @@ internal readonly partial struct ZipLocalFileHeader
         public static ReadOnlySpan<byte> SignatureConstantBytes => [0x50, 0x4B, 0x03, 0x04];
         public const int SizeOfLocalHeader = 30;
 
-        public static List<ZipGenericExtraField> GetExtraFields(Stream stream)
+        public static List<ZipGenericExtraField> GetExtraFields(Stream stream, out byte[] trailingData)
         {
             // assumes that TrySkipBlock has already been called, so we don't have to validate twice
 
@@ -538,8 +555,9 @@ public static List<ZipGenericExtraField> GetExtraFields(Stream stream)
                 stream.Seek(filenameLength, SeekOrigin.Current);
                 stream.ReadExactly(extraFieldBuffer);
 
-                result = ZipGenericExtraField.ParseExtraField(extraFieldBuffer);
+                result = ZipGenericExtraField.ParseExtraField(extraFieldBuffer, out ReadOnlySpan<byte> trailingDataSpan);
                 Zip64ExtraField.RemoveZip64Blocks(result);
+                trailingData = trailingDataSpan.ToArray();
 
                 return result;
             }
@@ -627,6 +645,7 @@ internal sealed partial class ZipCentralDirectoryFileHeader
         public byte[] Filename = [];
         public byte[] FileComment = [];
         public List<ZipGenericExtraField>? ExtraFields;
+        public byte[]? TrailingExtraFieldData;
 
         // if saveExtraFieldsAndComments is false, FileComment and ExtraFields will be null
         // in either case, the zip64 extra field info will be incorporated into other fields
@@ -718,14 +737,16 @@ public static bool TryReadBlock(ReadOnlySpan<byte> buffer, Stream furtherReads,
                 zip64 = new();
                 if (saveExtraFieldsAndComments)
                 {
-                    header.ExtraFields = ZipGenericExtraField.ParseExtraField(zipExtraFields);
+                    header.ExtraFields = ZipGenericExtraField.ParseExtraField(zipExtraFields, out ReadOnlySpan<byte> trailingDataSpan);
                     zip64 = Zip64ExtraField.GetAndRemoveZip64Block(header.ExtraFields,
                                 uncompressedSizeInZip64, compressedSizeInZip64,
                                 relativeOffsetInZip64, diskNumberStartInZip64);
+                    header.TrailingExtraFieldData = trailingDataSpan.ToArray();
                 }
                 else
                 {
                     header.ExtraFields = null;
+                    header.TrailingExtraFieldData = null;
                     zip64 = Zip64ExtraField.GetJustZip64Block(zipExtraFields,
                                 uncompressedSizeInZip64, compressedSizeInZip64,
                                 relativeOffsetInZip64, diskNumberStartInZip64);
diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs

Original file line number	Diff line number	Diff line change
`@@ -733,13 +733,17 @@ private void WriteFile()`
`733`	`733`	`if (entry.OffsetOfLocalHeader >= startingOffset)`
`734`	`734`	`{`
`735`	`735`	`// If the pending data to write is fixed-length metadata in the header, there's no need to load the compressed file bits.`
	`736`	`+ // We always need to load the local file header's metadata though - at this point, this entry will be written out and we`
	`737`	`+ // want to make sure that we preserve that metadata.`
`736`	`738`	`if ((entry.Changes & (ChangeState.DynamicLengthMetadata \| ChangeState.StoredData)) != 0)`
`737`	`739`	`{`
`738`	`740`	`completeRewriteStartingOffset = Math.Min(completeRewriteStartingOffset, entry.OffsetOfLocalHeader);`
`739`	`741`	`}`
	`742`	`+`
	`743`	`+ entry.LoadLocalHeaderExtraFieldIfNeeded();`
`740`	`744`	`if (entry.OffsetOfLocalHeader >= completeRewriteStartingOffset)`
`741`	`745`	`{`
`742`		`- entry.LoadLocalHeaderExtraFieldAndCompressedBytesIfNeeded();`
	`746`	`+ entry.LoadCompressedBytesIfNeeded();`
`743`	`747`	`}`
`744`	`748`
`745`	`749`	`entriesToWrite.Add(entry);`