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.

Sign up for GitHub

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

Jump to bottom

Read StartDiskNumber as uint32 #83923

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Read StartDiskNumber as uint32 #83923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/libraries/System.IO.Compression/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,6 @@
<data name="FieldTooBigOffsetToZip64EOCD" xml:space="preserve">
<value>Offset to Zip64 End Of Central Directory record cannot be held in an Int64.</value>
</data>
<data name="FieldTooBigStartDiskNumber" xml:space="preserve">
<value>Start Disk Number cannot be held in an Int64.</value>
</data>
<data name="FieldTooBigUncompressedSize" xml:space="preserve">
<value>Uncompressed Size cannot be held in an Int64.</value>
</data>
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public partial class ZipArchiveEntry
{
private ZipArchive _archive;
private readonly bool _originallyInArchive;
private readonly int _diskNumberStart;
private readonly uint _diskNumberStart;
private readonly ZipVersionMadeByPlatform _versionMadeByPlatform;
private ZipVersionNeededValues _versionMadeBySpecification;
internal ZipVersionNeededValues _versionToExtract;
Expand Down
9 changes: 4 additions & 5 deletions src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ internal struct Zip64ExtraField
private long? _uncompressedSize;
private long? _compressedSize;
private long? _localHeaderOffset;
private int? _startDiskNumber;
private uint? _startDiskNumber;

public ushort TotalSize => (ushort)(_size + 4);

Expand All @@ -115,7 +115,7 @@ internal struct Zip64ExtraField
get { return _localHeaderOffset; }
set { _localHeaderOffset = value; UpdateSize(); }
}
public int? StartDiskNumber => _startDiskNumber;
public uint? StartDiskNumber => _startDiskNumber;

private void UpdateSize()
{
Expand Down Expand Up @@ -242,7 +242,7 @@ private void UpdateSize()

if (readStartDiskNumber)
{
zip64Block._startDiskNumber = reader.ReadInt32();
zip64Block._startDiskNumber = reader.ReadUInt32();
}
else if (readAllFields)
{
Expand All @@ -253,7 +253,6 @@ private void UpdateSize()
if (zip64Block._uncompressedSize < 0) throw new InvalidDataException(SR.FieldTooBigUncompressedSize);
if (zip64Block._compressedSize < 0) throw new InvalidDataException(SR.FieldTooBigCompressedSize);
if (zip64Block._localHeaderOffset < 0) throw new InvalidDataException(SR.FieldTooBigLocalHeaderOffset);
if (zip64Block._startDiskNumber < 0) throw new InvalidDataException(SR.FieldTooBigStartDiskNumber);

return true;
}
Expand Down Expand Up @@ -480,7 +479,7 @@ internal struct ZipCentralDirectoryFileHeader
public ushort FilenameLength;
public ushort ExtraFieldLength;
public ushort FileCommentLength;
public int DiskNumberStart;
public uint DiskNumberStart;
public ushort InternalFileAttributes;
public uint ExternalFileAttributes;
public long RelativeOffsetOfLocalHeader;
Expand Down
164 changes: 164 additions & 0 deletions src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Buffers.Binary;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Xunit;
Expand Down Expand Up @@ -860,6 +861,20 @@ public void ReadArchive_WithUnexpectedZip64ExtraFieldSizeUncompressedSizeIn32Bit
Assert.Equal(6, entry.CompressedLength); // it should have used 32-bit size
}

/// <summary>
/// This test checks behavior of ZipArchive when the startDiskNumber in the extraField is greater than IntMax
/// </summary>
[Fact]
public void ReadArchive_WithDiskStartNumberGreaterThanIntMax()
{
byte[] input = (byte[])s_zip64WithBigStartDiskNumber.Clone();
using var archive = new ZipArchive(new MemoryStream(input));

var exception = Record.Exception(() => archive.Entries.First());

Assert.Null(exception);
danmoseley marked this conversation as resolved.
Show resolved Hide resolved
}

private static readonly byte[] s_slightlyIncorrectZip64 =
{
// ===== Local file header signature 0x04034b50
Expand Down Expand Up @@ -1000,5 +1015,154 @@ public void ReadArchive_WithUnexpectedZip64ExtraFieldSizeUncompressedSizeIn32Bit
// comment length
0x00, 0x00
};

private static readonly byte[] s_zip64WithBigStartDiskNumber =
Copy link
Member

Choose a reason for hiding this comment

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

Rather than duplicate this whole thing what about just flipping the byte you need, within the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, i'll try. But I changed quite a few bytes in different parts(https://www.diffchecker.com/YK5V7XvS), so any advice would be helpful for me how to do it better

Copy link
Member

Choose a reason for hiding this comment

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

I almost wonder then whether it would make sense to extract that "proto zip" data into a mock with a method like SetStartDiskNumber. That would make it cleaner to use and easier to maintain and extend.
It could in future eg abstract the ability to add more entries, which is more than just flipping some bytes, and maybe in future expose ways to get the bytes corrupted in interesting ways, without the tests knowing about the structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand your idea correctly. We take s_slightlyIncorrectZip64 as a basis and create extensions for it such as SetStartDiskNumber? (and smth like SetCustomHeaderField in future)

Copy link
Member

Choose a reason for hiding this comment

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

yes -- I mean, I created that for a particular test and I see it's getting used in another place, now you needed it here and had to make larger edits as you say. But those tests shouldn't need to know the zip format.

So I wonder whether that suggests it would be helpful to wrap the array in a class that exposes it and that class can take care of any modifications a test needs. For now if nothing else you could just paste your new array in there, alongside the other one, but it would be a start.

If you don't want to do that here, I think what you have is fine. (I'll let area owners sign off though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I don't have an idea for a good implementation of this right now. It is complicated by the fact that different tests use different initialization methods (zip files in ZipTestData, byte arrays through MemberData or static byte arrays)

In my vision, it would look something like this:

var zipFile = FluentZipFileGenerator.CreateBuilder()
		.GetDefaultZipTemplate()
		.SetZip64ExtraFieldSize(args)
		.SetStartDiskNumber(args)
		.GenerateZipFile();

In this PR I can move the archive to ZipTestData/StrangeZipFiles folder.

Copy link
Member

Choose a reason for hiding this comment

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

@dotnet/area-system-io-compression would have to say what they would like. I don't have any strong opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should tag someone else?

{
// ===== Local file header signature 0x04034b50
0x50, 0x4b, 0x03, 0x04,
// version to extract 4.5
0x2d, 0x00,
// general purpose flags
0x00, 0x08, // 0000_1000 'for enhanced deflating'
// Deflate
0x08, 0x00,
// Last mod file time
0x17, 0x9b,
// Last mod date
0x6d, 0x52,
// CRC32
0x0c, 0x7e, 0x7f, 0xd8,
// compressed size
0xff, 0xff, 0xff, 0xff,
// UNcompressed size
0xff, 0xff, 0xff, 0xff,
// file name length

0x08, 0x00,
// extra field length
0x20, 0x00,
// filename
0x66, 0x69, 0x6c, 0x65, 0x2e, 0x74, 0x78, 0x74,
// -----Zip64 extra field tag
0x01, 0x00,
// size of extra field block
0x20, 0x00,
// 8 byte Zip64 UNcompressed size, index 42
0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// 8 byte Zip64 compressed size, index 50
0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// 8 byte Relative Header Offset
0x0c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// Disk Start Number
0xff, 0xff, 0xff, 0xfe,
// ----- NTFS extra field tag
0x0a, 0x00,
// size of extra field block
0x20, 0x00,
// reserved
0x00, 0x00, 0x00, 0x00,
// tag #1
0x01, 0x00,
// size of tag #1
0x18, 0x00,
// Mtime, CTime, Atime
0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01,
0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01,
0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01,
// -------------
// Data!
0x2b, 0x49, 0x2d, 0x2e, 0x01, 0x00,
// -------- Central directory signature 0x02014b50
0x50, 0x4b, 0x01, 0x02,
// version made by 4.5
0x2d, 0x00,
// version to extract 4.5
0x2d, 0x00,
// general purpose flags
0x00, 0x08,
// Deflate
0x08, 0x00,
// Last mod file time
0x17, 0x9b,
// Last mod date
0x6d, 0x52,
// CRC32
0x0c, 0x7e, 0x7f, 0xd8,
// 4 byte compressed size, index 120 (-1 indicates refer to Zip64 extra field)
0xff, 0xff, 0xff, 0xff,
// 4 byte UNcompressed size, index 124 (-1 indicates refer to Zip64 extra field)
0xff, 0xff, 0xff, 0xff,
// file name length
0x08, 0x00,
// extra field length
0x44, 0x00,
// file comment length
0x00, 0x00,
// disk number start (-1 indicates refer to Zip64 extra field)
0xff, 0xff,
// internal file attributes
0x00, 0x00,
// external file attributes
0x00, 0x00, 0x00, 0x00,
// relative offset of local header (-1 indicates refer to Zip64 extra field)
0x00, 0x00, 0x00, 0x00,
// file name
0x66, 0x69, 0x6c, 0x65, 0x2e, 0x74, 0x78, 0x74,
// extra field, content similar to before
0x01, 0x00,
0x1c, 0x00,
0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// Disk start number
0xff, 0xff, 0xff, 0xfe,
0x0a, 0x00,
0x20, 0x00,
0x00, 0x00, 0x00, 0x00,
0x01, 0x00, 0x18, 0x00,
0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01,
0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01,
0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01,
// == 'end of zip64 central directory record' signature 0x06064b50
0x50, 0x4b, 0x06, 0x06,
// size
0x2c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// version made by, version needed
0x2d, 0x00, 0x2d, 0x00,
// disk number, disk number with CD
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// total number of CD records
0x01, 0x00, 0x00, 0x00,
// size of CD
0x00, 0x00, 0x00, 0x00,
// offset of start of CD wrt starting disk
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// zip64 extensible data sector
0x7a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// offset of start cd
0x70, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// == 'zip64 end of CD locator' signature 0x07064b50
0x50, 0x4b, 0x06, 0x07,
// number of disk with zip64 CD
0x00, 0x00, 0x00, 0x00,
// relative offset of zip64 ECD
0xea, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// total number of disks
0x01, 0x00, 0x00, 0x00,
// == 'end of CD' signature 0x06054b50
0x50, 0x4b, 0x05, 0x06,
// disk number, disk number with CD (-1 indicates refer to Zip64 extra field)
0x00, 0x00,
0x00, 0x00,
// total number of entries in CD on this disk, and overall (-1 indicates refer to Zip64 extra fields)
0xff, 0xff,
0xff, 0xff,
// size of CD (-1 indicates refer to Zip64 extra field)
0x7a, 0x00, 0x00, 0x00,
// offset of start of CD wrt start disk (-1 indicates refer to Zip64 extra field)
0x70, 0x00, 0x00, 0x00,
// comment length
0x00, 0x00
};
}
}