[tools] Support current and legacy assembly stores#12104
Conversation
Teach the assembly store reader to locate CoreCLR stores through the exported _assembly_store symbol while retaining the MonoVM payload-section fallback. Reuse the modern reader from decompress-assemblies without dropping legacy store support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55493571-86e2-4326-850e-ea55883aa5d8
There was a problem hiding this comment.
Pull request overview
This PR updates the inspection/decompression tooling to support the current CoreCLR assembly-store layout by locating the store via the exported _assembly_store ELF symbol (with a fallback to the legacy payload section), and wires that into decompress-assemblies so it can correctly extract assemblies from modern APK/AAB layouts under lib/{ABI}/libassembly-store.so. It also factors the v2 reader into a reusable library and ensures decompression failures propagate via the tool exit code.
Changes:
- Add ELF
_assembly_storesymbol lookup (withpayloadsection fallback) to the mk2 assembly-store reader. - Split the mk2 reader into a reusable
AssemblyStorelibrary and consume it from both the mk2 CLI anddecompress-assemblies. - Update
decompress-assembliesto handle both legacyassemblies/assemblies.bloband currentlib/{ABI}/libassembly-store.sostores, and to return non-zero on failures.
Show a summary per file
| File | Description |
|---|---|
| tools/decompress-assemblies/main.cs | Adds support for extracting from current lib/{ABI}/libassembly-store.so stores (plus better failure propagation), while keeping legacy store and individual-entry paths. |
| tools/decompress-assemblies/decompress-assemblies.csproj | Adds a project reference (with alias) to the mk2 AssemblyStore library and the Android SDK tools dependency needed for ABI/arch handling. |
| tools/assembly-store-reader-mk2/Main.cs | Removes redundant file-format/existence checks now handled by AssemblyStoreExplorer.Open. |
| tools/assembly-store-reader-mk2/Directory.Build.targets | Copies the launch script only for executable builds and makes the script path resolution robust via $(MSBuildThisFileDirectory). |
| tools/assembly-store-reader-mk2/AssemblyStore/Utils.cs | Implements _assembly_store dynamic symbol-based payload discovery with fallback to payload section scanning. |
| tools/assembly-store-reader-mk2/AssemblyStore/StoreReader_V2.cs | Uses the new ELF payload discovery API and improves the error message for invalid _assembly_store symbols. |
| tools/assembly-store-reader-mk2/AssemblyStore/ELFPayloadError.cs | Adds an error value for invalid payload symbols. |
| tools/assembly-store-reader-mk2/AssemblyStore/AssemblyStoreItem.cs | Makes AssemblyStoreItem public to support external consumers. |
| tools/assembly-store-reader-mk2/AssemblyStore/AssemblyStoreExplorer.cs | Makes AssemblyStoreExplorer public and updates the “no entries found” error message. |
| tools/assembly-store-reader-mk2/AssemblyStore/AssemblyStore.csproj | Introduces a dedicated reusable library project for the mk2 reader implementation. |
| tools/assembly-store-reader-mk2/assembly-store-reader.csproj | Switches the CLI to consume the new library project instead of compiling the sources directly. |
Copilot's findings
- Files reviewed: 11/11 changed files
- Comments generated: 0
|
/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.
⚠️ Needs Changes
Nice, well-structured follow-up that revives decompress-assemblies for symbol-based (libassembly-store.so) stores while preserving the legacy assemblies.blob and individual-entry paths, plus threading failures through the exit code. The ELF _assembly_store symbol resolution with a payload-section fallback is clean, and the 32/64-bit bounds checks are careful.
Issues (see inline comments):
⚠️ 1 error-handling:GetAndroidAbican throwNotSupportedExceptionforNone/Mips/Otherarchs (which pass theHasValueguard), crashing the tool instead of propagating a non-zero exit code — the opposite of one of this PR's goals.- 💡 2 suggestions: arch→ABI mapping duplicates
MonoAndroidHelper; and a diagnostic log for the zero-size symbol fallback.
Verification notes:
- CI is still pending on the head commit — not green yet, so not mergeable until checks pass.
- The retained
FindELFPayloadSectionOffsetAndSizewrapper is correctly kept for its build-task/test caller (ArchiveAssemblyHelper.cs), which now transparently gains symbol-based resolution. - Re: the PR's open question about deleting
decompress-assemblies— that's a reasonable call for maintainers; nothing in this diff blocks either decision.
No blocking correctness issues beyond the unhandled-exception path above.
Generated by Android PR Reviewer for #12104 · 112.5 AIC · ⌖ 18.8 AIC · ⊞ 6.8K
Comment /review to run again
Unify the legacy v1 parser with the modern reader, recognize v2 and v3 store layouts and archive names, and decode both XALZ/LZ4 and XAZS/Zstd assemblies. Add a standalone compatibility matrix and run it in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55493571-86e2-4326-850e-ea55883aa5d8
simonrozsival
changed the title
Use MonoAndroidHelper as the ABI source of truth, report unsupported store architectures without throwing, and log when zero-sized ELF symbols fall back to the containing section size. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55493571-86e2-4326-850e-ea55883aa5d8
Keep the file-app project removals while incorporating the shared ELF symbol diagnostic changes from the compatibility branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55493571-86e2-4326-850e-ea55883aa5d8
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Resolved in |
|
/review |
simonrozsival
added
the
ready-to-review
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Code review — ⚠️ minor suggestions
Reviewed the assembly-store tooling changes that add current/legacy store support, _assembly_store ELF-symbol detection, unified LZ4/Zstd decompression (AssemblyCompression), and per-arch v1 store reconstruction.
Assessment
Overall this is a solid, well-structured refactor. Highlights:
ReadEntryImageDatanow guards against short reads (nread == 0→EndOfStreamException) and returns pooled buffers infinally— a real robustness improvement over the previous unbounded loop.- V2 index parsing correctly distinguishes v2 (no
ignorebyte) from v3/v4 entry sizes, avoiding a mis-parse of the trailing byte. - Decompression logic is consolidated into
AssemblyCompressionand reused bytmt,decompress-assemblies, and the mk2 reader, eliminating three near-identical Zstd copies. - Good test coverage: both compression formats, v1 store sets/manifests/APK, and v2/v3 archive names & index layouts.
Comments (all 💡 suggestions, none blocking)
UncompressIfNeededthrowsNotSupportedExceptionfor all uncompress requests on the stable (non-net11) TFM, even for uncompressed entries — consider passing uncompressed data through instead.OpenV1fallback runs while theZipArchivehandle is still open on the same file — possible Windows sharing violation; scope theusingnarrower.uncompressedLengthfrom an untrusted header drives a poolRentup to ~2 GB before validation.
CI
No pipeline checks are reported yet on the head commit (status: pending). Not a blocker for this review — please confirm the Windows compatibility-test leg (added in build-windows-steps.yaml) goes green before merge.
Nice work overall.
Generated by Android PR Reviewer for #12104 · 156.2 AIC · ⌖ 12 AIC · ⊞ 6.8K
Comment /review to run again
| return stream; | ||
| #else | ||
| stream.Dispose (); | ||
| throw new NotSupportedException ("Assembly decompression requires .NET 11 or later"); |
There was a problem hiding this comment.
🤖 💡 API design — On the stable (non-NET11_0_OR_GREATER) target framework, UncompressIfNeeded throws NotSupportedException for every uncompressIfNeeded: true call, even when the entry is stored uncompressed. Since AssemblyStore.csproj multi-targets $(DotNetStableTargetFramework);$(DotNetTargetFramework), a net10.0 consumer that opens an uncompressed store and passes uncompressIfNeeded: true will fail unnecessarily. Consider only throwing when the data actually carries a compression magic (i.e. try to detect the header and pass through untouched otherwise), so uncompressed reads keep working on the stable TFM.
(Rule: Avoid surprising exceptions on supported code paths)
| } | ||
|
|
||
| reader.ReadUInt32 (); // descriptor index | ||
| uint uncompressedLength = reader.ReadUInt32 (); |
There was a problem hiding this comment.
🤖 💡 Error handling — uncompressedLength is read straight from the (untrusted) header and used as bytePool.Rent ((int)uncompressedLength). You already guard against > Int32.MaxValue, but a corrupt header can still request a very large allocation (up to ~2 GB) before any validation against the actual compressed size. For a diagnostics tool this is low-risk, but consider sanity-checking uncompressedLength against a plausible bound (or against compressedLength) so a malformed store fails fast instead of attempting a huge pool rent.
(Rule: Validate sizes from untrusted input before allocating)
| return (explorers, errorMessage); | ||
| } | ||
|
|
||
| (IList<AssemblyStoreExplorer>? legacyExplorers, string? legacyError) = OpenV1 (fi.FullName); |
There was a problem hiding this comment.
🤖 💡 Resource management — The OpenV1 fallback is invoked while the using var zip = ZipArchive.Open (fi.FullName, ...) handle from line 129 is still open, and OpenV1 → StoreReader_V1.Open re-opens the same file as a zip via the legacy explorer. On Windows this repo is sensitive to file-locking; a second open of the same archive while the first handle is live can fail depending on the share mode LibZipSharp requests. Consider closing/disposing zip before falling back to OpenV1 (e.g. scope the using to just the TryLoad call) to avoid a potential sharing violation.
(Rule: Beware concurrent handles to the same file on Windows)
Summary
_assembly_storeELF symbol while retaining the MonoVMpayloadsection fallbackXALZ/LZ4 andXAZS/Zstd compressed assembliesdecompress-assemblieswithout dropping direct DLL or individual-entry handlingThis follows up on #12033 (comment).
Compatibility
The reader now detects the archive layout, ELF wrapper, store format, and compression format independently:
assemblies/assemblies.blob;libassemblies.{abi}.blob.so;libassembly-store.sopayloadsection; exported_assembly_storesymbolXALZ/LZ4;XAZS/ZstdLegacy common assemblies are included in each ABI-specific view, matching the original store-set behavior.
Follow-up
A separate stacked PR will simplify the standalone tool projects into .NET file-based apps and introduce skills that invoke them for customer APK inspection and extraction. The shared compatibility parser remains in the repository because build/device tests and external projects consume or track it.
Validation
tmt