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

Fix Zip64ExtraField handling #111802

Merged
merged 2 commits into from
Jan 24, 2025
Merged

Conversation

edwardneal
Copy link
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

This comment was marked as resolved.

@carlossanlop
Copy link
Member

/azp list

Copy link

CI/CD Pipelines for this repository:

@carlossanlop
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop
Copy link
Member

  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal
Copy link
Contributor Author

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop
Copy link
Member

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop
Copy link
Member

  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants