Expose the ExternalAttributes field in ZipArchive #18565

ianhays · 2017-04-18T19:41:22Z

The ExternalFileAttributes field of a ZipArchiveEntry is currently hidden within the implementation and not accessible. This has been fine in the Windows world since it's not used but it is used by Unix's zip/unzip programs to store file permissions of an entry. As it currently stands, there is no way to carry Unix RWX permissions within a zip, so at unzip time the unzipper has to guess at permissions and manually chmod the entries. This PR simply exposes the raw field so that it may be used in any circumstance that requires access to the field. Tests are added to ensure that this field is set, read, and roundtrips properly with zips created from other common programs which use the field.

Note: Unix unzip doesn't apply permissions in the external file attributes field of a zip produced on Windows. So if you modify the externalfileattributes of a zip on Windows, unzip will use the defaults instead of what is set in the field. This isn't easily resolvable on our side since the platformmadeby byte determines a number of other things that we don't want to mess with (e.g. file name validity), so it would be unwise to lie about the platform on which the zip was written.

resolves https:/dotnet/corefx/issues/17067

cc: @eerhardt @danmosemsft @stephentoub

The ExternalFileAttributes field of a ZipArchiveEntry is currently hidden within the implementation and not accessible. This has been fine in the Windows world since it's not used but it is used by Unix's zip/unzip programs to store file permissions of an entry. As it currently stands, there is no way to carry Unix RWX permissions within a zip, so at unzip time the unzipper has to guess at permissions and manually chmod the entries. This PR simply exposes the raw field so that it may be used in any circumstance that requires access to the field. Tests are added to ensure that this field is set, read, and roundtrips properly with zips created from other common programs which use the field.

danmoseley · 2017-04-18T19:52:28Z

external/test-runtime/XUnit.Runtime.depproj

@@ -59,7 +59,7 @@
 <Version>2.5.0</Version>
 </PackageReference>
 <PackageReference Include="System.IO.Compression.TestData">
- <Version>1.0.4-prerelease</Version>
+ <Version>1.0.5-prerelease</Version>


you just manually upload this data when you need to?

Yeah. The TestData package doesn't need to be changed very often so it's not too much of a hassle.

danmoseley · 2017-04-18T19:55:36Z

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

@@ -39,6 +39,7 @@ public partial class ZipArchiveEntry
 private bool _currentlyOpenForWrite;
 private bool _everOpenedForWrite;
 private Stream _outstandingWriteStream;
+ private uint _externalFileAttr;


Is it still correct to use uint internally? Does it matter?
It's a bit odd to use it and then test with int.MaxValue.

It doesn't really matter. We could use an int internally instead, I just like the uint since you don't have to worry about bit carrying when doing shifts and it's already being using in the ZipCentralDirectoryFileHeader reader. Both could be changed to ints easily though.

danmoseley · 2017-04-18T19:57:59Z

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

@@ -527,7 +543,7 @@ internal void WriteCentralDirectoryFileHeader()
 writer.Write(_fileComment != null ? (ushort)_fileComment.Length : (ushort)0); // file comment length
 writer.Write((ushort)0); // disk number start
 writer.Write((ushort)0); // internal file attributes
- writer.Write((uint)0); // external file attributes
+ writer.Write(_externalFileAttr); // external file attributes


Is this line actually getting hit by your tests? Seems you're just reading zips you already made, and testing the set/get pairs.

It's not, really. Testing that the zips written work correctly with other programs requires manual testing (which FWIW I did).

As @eerhardt mentioned below, I can add a test to do a more complete roundtripping test to hit this line.

eerhardt · 2017-04-18T19:59:16Z

src/System.IO.Compression/tests/ZipArchive/zip_netcoreappTests.cs

+ [InlineData(int.MaxValue)]
+ [InlineData(int.MinValue)]
+ [InlineData(0)]
+ public static async Task RoundTrips_UnixFilePermissions(int expectedAttr)


I feel like we should have a test that actually writes the ExternalAttributes to a file, and then reads the file in and makes sure the ExternalAttributes are preserved in a full "round-trip".

Good point, I'll add one.

Added. There isn't any reason to write the stream to a file, so I closed the ZipArchive then reopened it to ensure the attribute was written to the stream correctly.

Good point. Just reading it back out of the stream works for me. Thanks.

eerhardt · 2017-04-18T20:05:03Z

src/System.IO.Compression/tests/System.IO.Compression.Tests.csproj

@@ -45,8 +49,11 @@
 <Link>Common\System\IO\Compression\ZipTestHelper.cs</Link>
 </Compile>
 </ItemGroup>
+ <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp'">


Is this new property not going to be supported in netstandard?

netstandard configuration means you run on netstandard. Adding the parameter is fine. I forgot to ask why you have to add a netcoreapp configuration here?

It would have to be added to full .net to be a part of netstandard, yes?

Right, but this has a netstandard configuration which indicates it works on top of netstandard. It can expose any API it wants. I don't think you need any new configuraiotn.

Disregard the above --this is a test configuration. I believe you did it right. My bad.

eerhardt · 2017-04-18T20:06:33Z

src/System.IO.Compression/tests/Configurations.props

@@ -4,6 +4,8 @@
 <BuildConfigurations>
 netstandard-Unix;
 netstandard-Windows_NT;
+ netcoreapp-Unix;


Does this mean we will build and run all the tests twice (with the exception of the newly added tests, which will only run once on netcoreapp)?

I'm not certain. As netstandard should be a subset of netcoreapp, it wouldn't make much sense to run a netstandard build alongside a netcoreapp build. My assumption is that when running tests against netcoreapp via build-tests.cmd, only the most specific Configuration is ran e.g. netcoreapp if available otherwise netstandard, or netfx where available otherwise netstandard. @karajas probably knows more.

Regardless, the important bit is that the new tests will run only on netcoreapp.

@ianhays is correct. We have https:/dotnet/corefx/issues/15667 to try and run explicitly the netstandard test configurations as well but in general those should be included in the netcoreapp configurations.

stephentoub

Other than Eric's feedback, LGTM.

eerhardt

Thanks for pushing forward with this @ianhays. This gets us closer to where we need to be to fully support non-Windows scenarios.

eerhardt · 2017-04-18T21:55:24Z

src/System.IO.Compression/tests/ZipArchive/zip_netcoreappTests.cs

+ [InlineData(int.MaxValue)]
+ [InlineData(int.MinValue)]
+ [InlineData(0)]
+ public static async Task RoundTrips_UnixFilePermissions(int expectedAttr)


Good point. Just reading it back out of the stream works for me. Thanks.

ianhays · 2017-04-19T19:34:56Z

Glad to help @eerhardt!

theladyjaye · 2018-03-12T12:06:47Z

ExternalAttributes appears to be available in .NET Core 2.x but it is not available in .NET Standard 2.x?

.NET Standard:
https://docs.microsoft.com/en-us/dotnet/api/system.io.compression.ziparchiveentry?view=netstandard-2.0

.NET Core
https://docs.microsoft.com/en-us/dotnet/api/system.io.compression.ziparchiveentry?view=netcore-2.0

weshaggard · 2018-03-12T19:07:05Z

@aventurella that is correct. This is a new API which is added to .NET Core first and will be considered in a future version of .NET Standard. The standard versions at a slower rate then .NET Core.

ianhays added api-approved API was approved in API review, it can be implemented area-System.IO.Compression labels Apr 18, 2017

ianhays added this to the 2.0.0 milestone Apr 18, 2017

ianhays self-assigned this Apr 18, 2017

dnfclas added the cla-already-signed label Apr 18, 2017

danmoseley reviewed Apr 18, 2017

View reviewed changes

eerhardt reviewed Apr 18, 2017

View reviewed changes

stephentoub approved these changes Apr 18, 2017

View reviewed changes

Add ZipArchive test for full roundtrip ExternAttr

9b67943

eerhardt approved these changes Apr 18, 2017

View reviewed changes

karelz removed the api-approved API was approved in API review, it can be implemented label Apr 18, 2017

Fix casing for Compression TestData package import

39e44d6

ianhays merged commit c2b9bca into dotnet:master Apr 19, 2017

tmds mentioned this pull request Apr 27, 2017

Executables restored from NuGet packages are no longer executable with .NET Core "2.0" NuGet/Home#4424

Closed

derwasp mentioned this pull request May 19, 2017

dotnet lambda deploy-serverless on Debian Jessie produces a non working function aws/aws-lambda-dotnet#121

Closed

slang25 mentioned this pull request Mar 3, 2018

Use dotnet compression on .NET Core 2.0^ for all platforms aws/aws-lambda-dotnet#238

Closed

This was referenced Mar 12, 2018

Support ExternalAttributes for ZipArchives adamhathcock/sharpcompress#357

Open

Support ExternalAttributes in System.IO.Compression.ZipArchiveEntry dotnet/standard#673

Closed

ianhays deleted the zipexternattr branch June 11, 2018 17:59

eerhardt mentioned this pull request Jan 9, 2020

Compression.ZipFile support for Unix Permissions dotnet/runtime#1548

Closed

snakefoot mentioned this pull request Jun 21, 2020

Zip Archive created under Linux is missing file permissions NLog/NLog#4022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose the ExternalAttributes field in ZipArchive #18565

Expose the ExternalAttributes field in ZipArchive #18565

ianhays commented Apr 18, 2017

danmoseley Apr 18, 2017

ianhays Apr 18, 2017

danmoseley Apr 18, 2017

ianhays Apr 18, 2017

danmoseley Apr 18, 2017

ianhays Apr 18, 2017

eerhardt Apr 18, 2017

ianhays Apr 18, 2017

ianhays Apr 18, 2017

eerhardt Apr 18, 2017

eerhardt Apr 18, 2017

danmoseley Apr 18, 2017

ianhays Apr 18, 2017

danmoseley Apr 18, 2017

danmoseley Apr 18, 2017

eerhardt Apr 18, 2017

ianhays Apr 18, 2017

weshaggard Apr 18, 2017

stephentoub left a comment

eerhardt left a comment

eerhardt Apr 18, 2017

ianhays commented Apr 19, 2017

theladyjaye commented Mar 12, 2018

weshaggard commented Mar 12, 2018

Expose the ExternalAttributes field in ZipArchive #18565

Expose the ExternalAttributes field in ZipArchive #18565

Conversation

ianhays commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianhays commented Apr 19, 2017

theladyjaye commented Mar 12, 2018

weshaggard commented Mar 12, 2018