Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting ZipArchiveEntry general-purpose flag bits #98278

Merged
merged 7 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd)

_fileComment = cd.FileComment;

_compressionLevel = null;
_compressionLevel = MapCompressionLevel(_generalPurposeBitFlag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place I found reading the value of _compressionLevel that could potentially change behavior due to this change is inside the GetDataCompressor method (in this same file), in the default switch case, where if the value of _compressionLevel is null, we default to CompressionLevel.Optimal.

Can you please double check if we have a test that can reach the code inside the switch's default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the better option here might be to eliminate the null check. The only time that _compressionLevel is going to be null is when creating a new ZipArchiveEntry. This sets _compressionLevel to null, then sets the bit flags to zero in MapDeflateCompressionOption. GetDataCompressor will then interpret the null as CompressionLevel.Optimal implicitly.

If the same ZIP file is loaded again though, MapCompressionLevel will map the zero to CompressionLevel.Optimal explicitly. I think it'd be clearer to make _compressionLevel non-nullable, set it to CompressionLevel.Optimal explicitly in the constructor and remove the coalesce inside GetDataCompressor.

Doing this means that there's no extra check in GetDataCompressor's default case for a test to reach. There's provably no behavioural change here to test because the current behaviour is effectively:

_compressionLevel = null;
// ...
compressorStream = new DeflateStream(/**/, _compressionLevel ?? CompressionLevel.Optimal, /**/);

while the new behaviour would be:

_compressionLevel = CompressionLevel.Optimal;
// ...
compressorStream = new DeflateStream(/**/, _compressionLevel, /**/);

}

// Initializes a ZipArchiveEntry instance for a new archive entry with a specified compression level.
Expand All @@ -98,6 +98,7 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName, CompressionLevel
{
CompressionMethod = CompressionMethodValues.Stored;
}
_generalPurposeBitFlag = MapDeflateCompressionOption(_generalPurposeBitFlag, _compressionLevel);
}

// Initializes a ZipArchiveEntry instance for a new archive entry.
Expand All @@ -111,7 +112,8 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName)
_versionMadeByPlatform = CurrentZipPlatform;
_versionMadeBySpecification = ZipVersionNeededValues.Default;
_versionToExtract = ZipVersionNeededValues.Default; // this must happen before following two assignment
_generalPurposeBitFlag = 0;
_compressionLevel = null;
_generalPurposeBitFlag = MapDeflateCompressionOption(0, _compressionLevel);
edwardneal marked this conversation as resolved.
Show resolved Hide resolved
CompressionMethod = CompressionMethodValues.Deflate;
_lastModified = DateTimeOffset.Now;

Expand All @@ -138,8 +140,6 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName)

_fileComment = Array.Empty<byte>();

_compressionLevel = null;

if (_storedEntryNameBytes.Length > ushort.MaxValue)
throw new ArgumentException(SR.EntryNamesTooLong);

Expand Down Expand Up @@ -799,6 +799,35 @@ private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out st

private bool SizesTooLarge() => _compressedSize > uint.MaxValue || _uncompressedSize > uint.MaxValue;

private static CompressionLevel? MapCompressionLevel(BitFlagValues generalPurposeBitFlag)
{
// Information about the Deflate compression option is stored in bits 1 and 2 of the general purpose bit flags.
int deflateCompressionOption = (int)generalPurposeBitFlag & 0x6;

return deflateCompressionOption switch
{
0 => CompressionLevel.Optimal,
2 => CompressionLevel.SmallestSize,
4 => CompressionLevel.Fastest,
6 => CompressionLevel.NoCompression,
_ => null
};
}

private static BitFlagValues MapDeflateCompressionOption(BitFlagValues generalPurposeBitFlag, CompressionLevel? compressionLevel)
{
ushort deflateCompressionOptions = compressionLevel switch
edwardneal marked this conversation as resolved.
Show resolved Hide resolved
{
CompressionLevel.Optimal => 0,
CompressionLevel.SmallestSize => 2,
CompressionLevel.Fastest => 4,
CompressionLevel.NoCompression => 6,
_ => 0
};

return (BitFlagValues)(((int)generalPurposeBitFlag & ~0x6) | deflateCompressionOptions);
}

// return value is true if we allocated an extra field for 64 bit headers, un/compressed size
private bool WriteLocalFileHeader(bool isEmptyFile)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,77 @@ public static void CreateUncompressedArchive()
}
}

// This test checks to ensure that setting the compression level of an archive entry sets the general-purpose
// bit flags correctly. It verifies that these have been set by reading from the MemoryStream manually, and by
// reopening the generated file to confirm that the compression levels match.
[Theory]
[InlineData(CompressionLevel.NoCompression, 6)]
[InlineData(CompressionLevel.Optimal, 0)]
[InlineData(CompressionLevel.SmallestSize, 2)]
[InlineData(CompressionLevel.Fastest, 4)]
public static void CreateArchiveEntriesWithBitFlags(CompressionLevel compressionLevel, ushort expectedGeneralBitFlags)
{
var testfilename = "testfile";
var testFileContent = "Lorem ipsum dolor sit amet, consectetur adipiscing elit.";
var utf8WithoutBom = new Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false);

byte[] zipFileContent;

using (var testStream = new MemoryStream())
{

using (var zip = new ZipArchive(testStream, ZipArchiveMode.Create))
{
ZipArchiveEntry newEntry = zip.CreateEntry(testfilename, compressionLevel);
using (var writer = new StreamWriter(newEntry.Open(), utf8WithoutBom))
{
writer.Write(testFileContent);
writer.Flush();
}

ZipArchiveEntry secondNewEntry = zip.CreateEntry(testFileContent + "_post", CompressionLevel.NoCompression);
}

zipFileContent = testStream.ToArray();
}

// expected bit flags are at position 6 in the file header
var generalBitFlags = System.Buffers.Binary.BinaryPrimitives.ReadUInt16LittleEndian(zipFileContent.AsSpan(6));

Assert.Equal(expectedGeneralBitFlags, generalBitFlags);

using (var reReadStream = new MemoryStream(zipFileContent))
{
using (var reReadZip = new ZipArchive(reReadStream, ZipArchiveMode.Read))
{
var firstArchive = reReadZip.Entries[0];
var secondArchive = reReadZip.Entries[1];
var compressionLevelFieldInfo = typeof(ZipArchiveEntry).GetField("_compressionLevel", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
var generalBitFlagsFieldInfo = typeof(ZipArchiveEntry).GetField("_generalPurposeBitFlag", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);

var reReadCompressionLevel = (CompressionLevel)compressionLevelFieldInfo.GetValue(firstArchive);
var reReadGeneralBitFlags = (ushort)generalBitFlagsFieldInfo.GetValue(firstArchive);

Assert.Equal(compressionLevel, reReadCompressionLevel);
Assert.Equal(expectedGeneralBitFlags, reReadGeneralBitFlags);

reReadCompressionLevel = (CompressionLevel)compressionLevelFieldInfo.GetValue(secondArchive);
Assert.Equal(CompressionLevel.NoCompression, reReadCompressionLevel);

using (var strm = firstArchive.Open())
{
var readBuffer = new byte[firstArchive.Length];

strm.Read(readBuffer);

var readText = Text.Encoding.UTF8.GetString(readBuffer);

Assert.Equal(readText, testFileContent);
}
}
}
}

[Fact]
public static void CreateNormal_VerifyDataDescriptor()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,11 @@ internal static void GetZipCompressionMethodFromOpcCompressionOption(
break;
case CompressionOption.Maximum:
{
#if (!NETSTANDARD2_0 && !NETFRAMEWORK)
compressionLevel = CompressionLevel.SmallestSize;
#else
compressionLevel = CompressionLevel.Optimal;
#endif
edwardneal marked this conversation as resolved.
Show resolved Hide resolved
}
break;
case CompressionOption.Fast:
Expand All @@ -393,7 +397,7 @@ internal static void GetZipCompressionMethodFromOpcCompressionOption(
break;
case CompressionOption.SuperFast:
{
compressionLevel = CompressionLevel.Fastest;
compressionLevel = CompressionLevel.NoCompression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change, but if it's more appropriate, we can take it.

@edwardneal @maedula is this the preferred/more correct CompressionLevel value when converting from XPS?

}
break;

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.IO.Packaging/tests/ReflectionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public void Verify_GeneralPurposeBitFlag_NotSetTo_Unicode()
FieldInfo fieldInfo = typeof(ZipArchiveEntry).GetField("_generalPurposeBitFlag", BindingFlags.Instance | BindingFlags.NonPublic);
object fieldObject = fieldInfo.GetValue(entry);
ushort shortField = (ushort)fieldObject;
Assert.Equal(0, shortField); // If it was UTF8, we would set the general purpose bit flag to 0x800 (UnicodeFileNameAndComment)
Assert.Equal(0, shortField & 0x800); // If it was UTF8, we would set the general purpose bit flag to 0x800 (UnicodeFileNameAndComment)
edwardneal marked this conversation as resolved.
Show resolved Hide resolved
CheckCharacters(entry.Name);
CheckCharacters(entry.Comment); // Unavailable in .NET Framework
}
Expand Down
Loading