Repository navigation
[Xamarin.Installer.AndroidSDK] Drop SharpZipLib - #13464
simonrozsival wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The parser rejects valid ZIP archives containing an optional central-directory digital signature, and the new tests do not exercise the custom ZIP64 parsing path.
2 open issues
What changed in this PR
Replaces SharpZipLib in the Android SDK installer with .NET ZIP APIs, removing the dependency without the framework retargeting planned in #12988.
Changes:
- Uses streaming extraction with progress reporting and retains the existing path traversal check.
- Adds central-directory metadata parsing for timestamps and attributes, with extraction and ZIP64 progress tests.
- Removes SharpZipLib package references, its version property, and installer packaging entries.
| File | Description |
|---|---|
tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj |
Removes the SharpZipLib test dependency. |
tests/Xamarin.Android.Tools.AndroidSdk-Tests/AndroidSdkInstallerTests.cs |
Adds extraction, metadata, traversal, and ZIP64 progress tests. |
src/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj |
Removes SharpZipLib package and symbol-copy references. |
src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK/DirectorySizeMonitoringTimer.cs |
Uses .NET ZIP APIs for size and entry counts. |
src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK/Common/ZipCentralDirectory.cs |
Reads supplementary ZIP metadata and ZIP64 directory locations. |
src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK/Common/Helper.cs |
Migrates extraction and metadata restoration; strips special Unix permission bits. |
src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK.csproj |
Removes the installer’s SharpZipLib dependency. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj |
Removes the build-test SharpZipLib reference. |
Directory.Build.props |
Removes the SharpZipLib version property. |
build-tools/installers/create-installers.targets |
Stops packaging SharpZipLib binaries and symbols. |
Review effort: Balanced
| while (reader.BaseStream.Position < centralDirectoryEnd) { | ||
| if (centralDirectoryEnd - reader.BaseStream.Position < CentralDirectoryHeaderSize) | ||
| throw new InvalidDataException ("Unexpected end of ZIP central directory."); | ||
| if (entries.Count == expectedEntryCount) | ||
| throw new InvalidDataException ("ZIP central directory contains too many entries."); |
| if (centralDirectorySize == uint.MaxValue || centralDirectoryOffset == uint.MaxValue) | ||
| ReadZip64CentralDirectoryLocation (reader, endOfCentralDirectoryPosition, out centralDirectoryOffset, out centralDirectorySize); |
Xamarin.Installer.AndroidSDK used ICSharpCode.SharpZipLib only to
extract SDK package archives. Use System.IO.Compression.ZipArchive
for decompression, plus a small bounds-checked central directory
reader (ZipCentralDirectory) to recover the metadata that the
netstandard2.0 ZipArchive API doesn't expose:
* "version made by" host system (DOS attributes on Windows only for
DOS/NTFS-created entries, as before),
* external file attributes (Unix permission bits for chmod),
* extended Unix timestamp (0x5455) mtime, and SharpZipLib's
entry-name decoding and directory detection.
Entries are cross-checked against ZipArchive (count and sizes) before
extraction. Zip-slip and long-path handling are unchanged.
Remove the SharpZipLib PackageReference, installer packaging entries
and the SharpZipLibVersion property.
Verified on 7 real SDK archives (build-tools r36 mac/windows,
platform-tools r37.0.1, platform-37.2, cmake 4.1.2, cmdline-tools,
source-37.2): identical file contents, sizes, modes, attributes and
mtimes vs. main. Extraction is 1.2-2.3x faster on .NET 10.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extract entry data with a validating reader instead of ZipArchive so the installer keeps SharpZipLib 1.3.3's extract-time checks: local header signature, version/flags, local vs. central size consistency (including ZIP64 and data descriptors), encrypted entries and truncated deflate data. Entry output must also match the central directory size. The validation logic is derived from SharpZipLib (MIT), so add its notice to THIRD-PARTY-NOTICES.TXT. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: db33386 The earlier SharpZipLib replacement implemented a separate ZIP reader to reproduce metadata and local-header validation. This substantially increases the amount of archive-handling code maintained by the installer. Use System.IO.Compression.ZipArchive directly for extraction and entry counting, retaining the existing parent-traversal check. Delete an existing destination before extraction and restore Unix permissions using Mono.Unix so duplicate entries, mode-zero entries, and files lacking owner-read permissions retain the expected modes. Strip set-ID and sticky permission bits. Access the entry attributes through a cached property lookup because the netstandard2.0 reference API does not expose them. Restore coverage for data descriptors and ZIP64 entry counts. Remove the custom reader and its SharpZipLib-derived code and notices. This is not a behavior-equivalent replacement: SharpZipLib's local-header checks and support for some ZIP variants are no longer preserved, Windows DOS attributes are not restored, and the public CalculateTotalSize parameter type changes. These compatibility differences require review before the change is shipped. Eight counterbalanced rounds on seven SDK archives favored the candidate median for six archives; CMake was about 9% slower on the shared host. The extracted files matched the baseline in content, mode, attributes, and modification time on macOS, excluding nondeterministic directory timestamps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Some Android system-image archives consist almost entirely of one multi-gigabyte file. Reporting progress only after each entry is extracted leaves the installer apparently stalled for most of the operation. Read each ZipArchiveEntry through the built-in stream API and copy with a reusable buffer, reporting progress as data is written. Keep the existing destination-file and Unix permission handling without introducing a ZIP metadata reader. A 32 MiB one-entry repro reports intermediate progress again; the previous candidate reported only 0% and 100%. The existing installer tests pass, and seven SDK archives match baseline extraction contents and metadata on macOS. This replacement remains conditional: ZipArchive does not expose extended Unix timestamps, and a checksum-verified approved Google archive extracts with different timestamps than SharpZipLib. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use ZipArchive for streaming extraction and read only central-directory metadata that ZipArchiveEntry does not expose. Preserve extended timestamps, creator-specific attributes, Unix permissions, and DOS directory markers while keeping the existing traversal guard and progress reporting. BZip2, prefixed ZIP, and local-header validation support from SharpZipLib is not retained; the approved Android SDK feed archives do not use those features. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
c67efaa to
2b81f2f
Compare
Context: #13452 The rebase conflict resolution restored Polly package references even though main had already replaced the installer retry policy. Remove those references and the unused version property; the retry implementation remains unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #13464 ZIP64 and comment records can cause ZipArchive and the metadata reader to select different central directories. Reject conflicting records before applying attributes to extracted files, and use ordinal path containment with checks for existing reparse points. The NDK archives contain Unix symbolic links. Extract validated in-tree links after regular files and preserve them when copying the component. Use private Unix staging directories and remove group and other write permissions from extracted regular files. These checks require a trusted destination: concurrent changes to an existing extraction directory can still race path validation. Some previously accepted archive layouts and symbolic links are now rejected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #13464 A second security review of 1f5f04d found four issues in the new symbolic link support: * NDK r23b (23.1.7779620) failed to install on macOS and Linux. Its four `bin/as` entries are symlinks to absolute paths on Google's build machines, which the new validation rejected. Entries with absolute targets are once again stored as regular files containing the target text, as SharpZipLib did. * A relative target could climb above the temporary install root with leading `..` and re-enter through the root's archive-controlled name. Once the component was moved, the link pointed into a sibling SDK component, and writing `package.xml` through it overwrote that component's file. Leading `..` segments may no longer climb above the install root, and `GeneratePackageXml()` deletes an existing link before writing. * The missing/cyclic target check never fired, because `File.Exists()` uses `lstat()` on Unix. Use `stat()`, which follows the link. * Windows threw `PlatformNotSupportedException` for Unix symlink entries. These are again stored as regular files, as before. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: #13464 The ZIP tests added by this PR had grown to 21 cases with repeated temp-directory and archive boilerplate. Keep only tests that guard behavior this PR changes, and consolidate overlapping ones: * Fold the set-ID, group/other write, and unspecified-mode cases into `UnzipPreservesContentsTimestampsAndPermissions`. * Merge the `../` and case-variant traversal tests into `UnzipRejectsPathsOutsideDestination`. * Fold the absolute symlink target (NDK r23b) case into `UnzipAndMovePreserveUnixSymbolicLinks`. * Merge the valid ZIP64, conflicting ZIP64, and end-record-in-comment cases into `UnzipRequiresUnambiguousCentralDirectory`. * Drop the reflection-based `TemporaryDirectory` test, the empty-archive top-level symlink edge case, and the reflection added to `CalculateMoveProgressSplitSupportsZip64Archives`. Unix-only tests now set modes through `ZipArchiveEntry.ExternalAttributes` instead of patching the central directory. Reverting the permission mask or the ordinal containment check still fails the consolidated tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Associate the all-bits-set unspecified mode with Unix ZIP metadata instead of treating it as DOS directory attributes. Exercise a normal DOS entry as well. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code assessment: no merge-blocking defects found. Reviewed the diff and all 15 changed files, including extraction callers, package relocation/cleanup, target-framework constraints, dependency removal, and the new fixtures. Findings: 0 errors, 0 warnings, 2 non-blocking suggestions, posted inline for actual ZIP64 count-overflow extraction coverage and Windows handling of Unix-symlink entries.
The bounded metadata reader, explicit permission handling, and extraction/move regression tests are useful improvements. The dependency removal is consistent across the affected projects and installer file list; the PR description explicitly identifies the intentional compatibility changes.
dotnet-android check for Azure build 1674857. Direct Azure build-result/timeline inspection was blocked because az devops invoke requires login in this environment, so I cannot independently confirm the Azure result. A focused dotnet test attempt for AndroidSdkInstallerTests, using net10.0 overrides for the available SDK, failed during CLI initialization with an IOException creating the NuGet-Migrations mutex, before tests ran. These are review-environment limits, not demonstrated PR failures.
Generated by Android PR Reviewer for #13464 · copilot · gpt61 · 50.4 AIC · ⌖ 4.89 AIC · ⊞ 20.2K
Comment /review to run again
| zip.CreateEntry ($"f{i}"); | ||
| } | ||
|
|
||
| Assert.AreEqual (50, DirectorySizeMonitoringTimer.CalculateMoveProgressSplit (temp.ZipPath)); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider also covering extraction of a ZIP64 archive whose actual entry count exceeds 65,535. This test exercises only CalculateMoveProgressSplit, which uses the framework reader; UnzipRequiresUnambiguousCentralDirectory exercises the new metadata reader with a one-entry ZIP64 fixture. Neither currently checks that ZipCentralDirectory.Read accepts the real count-overflow case and keeps its metadata aligned with ZipArchive.Entries.
{Rule: Test edge cases}
| } | ||
| #if !WINDOWS | ||
| // On Windows, and for absolute targets, keep the previous behavior: store the target text as a regular file. | ||
| if (metadata.IsUnixSymlink && !Platform.IsWindows) { |
There was a problem hiding this comment.
🤖 💡 Testing — Could you add a Windows-only regression test for the documented Unix-symlink fallback? Force creator platform 3 and symlink mode with the existing SetUnixAttributes helper, extract on Windows, and assert that the result is a regular file containing the target text. The current symlink tests skip Windows, and archives created by WriteEntry on Windows have Windows creator metadata, so those fixtures would not establish this compatibility behavior without the explicit metadata override.
{Rule: Test platform-specific behavior}
simonrozsival
added
the
ready-to-review
|
@dalexsoto review |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
Reviewed all 15 changed files statically at head 094deee, covering ZIP/ZIP64 central-directory parsing bounds, path containment and symlink/reparse-point handling, permission stripping, temp-directory handling, callers of the changed CalculateTotalSize signature, target-framework API availability, packaging/csproj removals and tests, plus a separate final search for additional blockers. No blocking issue found; the documented compatibility tradeoffs are accepted. Non-blocking note: the strict parser rejects the rare central-directory digital-signature record (SecureZIP-signed archives), which is a possible follow-up. Static review only: no code was executed, so Windows, native Linux and .NET Framework MSBuild behavior are not independently validated.
Context: The Android SDK installer uses SharpZipLib for decompression and metadata that
ZipArchiveEntrydoes not expose on its target framework. A built-in-only extraction path lost extended Unix timestamps and Windows DOS attributes.Changes: Use
ZipArchivefor streaming decompression and a bounded central-directory reader for timestamps, creator platform, and external attributes. Remove SharpZipLib from the installer, build tasks, tests, and installer packaging.Two security review rounds shaped the extraction hardening:
package.xmlis never written through a link.bin/asentries in NDK r23b), and all Unix symlink entries on Windows, are stored as regular files containing the target text, as SharpZipLib did.Compatibility:
CalculateTotalSizepublic signature changes fromZipFiletoZipArchive.This change does not include the framework retargeting in #12988, which updates some of the same project files. Dependency removal is the goal; no performance improvement is claimed.
Validation on macOS, using a session-local clone of the .NET SDK
12.0.100-alpha.1.26551.101with private HOME, CLI, NuGet, XDG, and temp state:dotnet test tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj --no-restore -v minimal -p:AndroidToolsDisableMultiTargeting=false -p:DotNetTargetFrameworkVersion=11.0— 564 passed, 7 skipped (net11.0). An unrelated fake-emulator test failed once, then passed on retry. The permissions fixture covers Unix and DOS creator metadata; native Windows execution was not available locally.dotnet build src/Xamarin.Installer.Build.Tasks/Xamarin.Installer.Build.Tasks.csproj -v quiet— passed.dotnet build src/Xamarin.Installer.AndroidSDK/Xamarin.Installer.AndroidSDK.csproj -v quiet -p:DefineConstants=WINDOWS— passed as a Windows-conditional compile, not Windows execution.Native Linux and Windows execution, .NET Framework MSBuild, and full installer packaging remain unvalidated.