[monodroid] Properly process satellite assemblies #7823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes: dotnet#7819 Our native runtime uses a cache of pointers to loaded managed assembly images (essentially an array of the native struct `MonoImage` pointers) which is pre-allocated at build time and placed in the native `libxamarin-app.so` library. While generating the cache, we also generate hashes for a number of assembly name permutations (currently two per assembly - with and without the extension). Only unique assembly names are considered when generating the cache (it's possible to have duplicate names, because we package more than one copy of some assemblies - those which are architecture specific). This algorithm had a bug which made it ignore culture prefix in satellite assembly names (e.g. `en/MyAssembly.resources.dll`) and instead of several entries for each culture, we generated only two entries for `MyAssembly.resources.dll` and `MyAssembly.resources` but we still counted each culture-prefixed assembly and stored that number in `libxamarin-app.so` to be used at runtime to calculate number of entries in the cache. This made the array storing cached `MonoImage*` pointers to be smaller than the number of actual assemblies in the APK times 2 and in some cases we failed to look up pointer to some images and, as the result, passed a `NULL` pointer to MonoVM which then caused a segmentation fault trying to dereference the pointer. Stop ignoring the culture prefix for satellite assemblies in order to avoid the situation. Additionally, since the previous assumption that MonoVM will validate all pointers passed to its APIs turned out to be unwarranted, we now check more carefully for `NULL` pointers when trying to obtain a native function pointer from the MonoVM runtime.
grendello
requested review from
jonpryor,
dellis1972 and
jonathanpeppers
as code owners
Comment on lines
+268
to
+270
| // We need to use the 'RelativePath' metadata, if found, because it will give us the correct path for satellite assemblies - with the culture in the path. | ||
| string? relativePath = assembly.GetMetadata ("RelativePath"); | ||
| string assemblyName = String.IsNullOrEmpty (relativePath) ? Path.GetFileName (assembly.ItemSpec) : relativePath; |
There was a problem hiding this comment.
Do you have a .binlog of what was happening here?
In other places we used %(DestinationSubDirectory) because it was item metadata we create & control what the value is:
I think we should just check which one we should use everywhere. /cc @dellis1972
There was a problem hiding this comment.
These are the relevant entries for statellite assemblies as passed to GeneratePackageManagerJava. DestinationDirectory or DestinationSubPath could be used as well, but I figured RelativePath was a standard MSBuild metadata item?
Task Parameter:
ResolvedUserAssemblies=
~/.nuget/packages/microsoft.maui.controls.core/8.0.0-preview.1.7762/lib/net8.0-android33.0/ar/Microsoft.Maui.Controls.resources.dll
AdditionalProperties=RuntimeIdentifier=android-arm;
_ComputeFilesToPublishForRuntimeIdentifiers=true
;AppendRuntimeIdentifierToOutputPath=true
;SkipCompilerExecution=true
;_OuterIntermediateAssembly=obj/Release/net8.0-android/issue7819.dll
;_OuterOutputPath=bin/Release/net8.0-android/
;_OuterIntermediateOutputPath=obj/Release/net8.0-android/
AssetType=resources
CopyLocal=true
CopyToPublishDirectory=PreserveNewest
Culture=ar
DestinationSubDirectory=ar/
DestinationSubPath=ar/Microsoft.Maui.Controls.resources.dll
FrameworkAssembly=False
HasMonoAndroidReference=False
MSBuildSourceProjectFile=~/.../issue7819/issue7819.csproj
MSBuildSourceTargetName=_ComputeFilesToPublishForRuntimeIdentifiers
NuGetPackageId=Microsoft.Maui.Controls.Core
NuGetPackageVersion=8.0.0-preview.1.7762
OriginalItemSpec=issue7819.csproj
PathInPackage=lib/net8.0-android33.0/ar/Microsoft.Maui.Controls.resources.dll
RelativePath=ar/Microsoft.Maui.Controls.resources.dll
RuntimeIdentifier=android-arm
~/.nuget/packages/microsoft.maui.controls.core/8.0.0-preview.1.7762/lib/net8.0-android33.0/ca/Microsoft.Maui.Controls.resources.dll
AdditionalProperties=RuntimeIdentifier=android-arm;
_ComputeFilesToPublishForRuntimeIdentifiers=true
;AppendRuntimeIdentifierToOutputPath=true
;SkipCompilerExecution=true
;_OuterIntermediateAssembly=obj/Release/net8.0-android/issue7819.dll
;_OuterOutputPath=bin/Release/net8.0-android/
;_OuterIntermediateOutputPath=obj/Release/net8.0-android/
AssetType=resources
CopyLocal=true
CopyToPublishDirectory=PreserveNewest
Culture=ca
DestinationSubDirectory=ca/
DestinationSubPath=ca/Microsoft.Maui.Controls.resources.dll
FrameworkAssembly=False
HasMonoAndroidReference=False
MSBuildSourceProjectFile=~/.../issue7819/issue7819.csproj
MSBuildSourceTargetName=_ComputeFilesToPublishForRuntimeIdentifiers
NuGetPackageId=Microsoft.Maui.Controls.Core
NuGetPackageVersion=8.0.0-preview.1.7762
OriginalItemSpec=issue7819.csproj
PathInPackage=lib/net8.0-android33.0/ca/Microsoft.Maui.Controls.resources.dll
RelativePath=ca/Microsoft.Maui.Controls.resources.dll
RuntimeIdentifier=android-arm
There was a problem hiding this comment.
Note that it isn't just a path - it's the de-facto assembly name. The runtime will ask us to load an assembly named culture/Assembly.dll, so we absolutely must keep that convention
There was a problem hiding this comment.
I think we should be ok to use RelativePath here, but DestinationSubPath should also work as we use that for fast deployment.
There was a problem hiding this comment.
We have some logic that sets %(DestinationSubPath) if the assembly is architecture-specific:
I don't think %(RelativePath) would have a value for armeabi-v7a/Assembly.dll.
There was a problem hiding this comment.
@jonathanpeppers yeah, %(RelativePath) didn't have the arch-specific prefix
|
@grendello: standard-request-is-standard: icanhaz unit test? kthxbye! :-D |
Add a test for the issue, based on the
`MissingSatelliteAssemblyInLibrary` packaging test which, when it fails,
will result in a `SIGABRT`:
D monodroid-assembly: assembly_store_open_from_bundles: looking for bundled name: 'System.Private.CoreLib' (hash 0x6b0ff375198b9c17)
F monodroid-assembly: Invalid assembly index 19, exceeds the maximum index of 11
F libc : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 17957 (semblyinlibrary), pid 17957 (semblyinlibrary)
Enhance the packaging test `MissingSatelliteAssemblyInLibrary` by adding
more languages, to see if they're all packaged correctly.
* main: Localized file check-in by OneLocBuild (dotnet#7827) Revert "Bump to dotnet/installer@d25a3bb 8.0.100-preview.2.23105.6 (dotnet#7769)" LEGO: Merge pull request 7828 LEGO: Merge pull request 7825 Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799) Bump NDK to r25c (dotnet#7808) [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816) [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
Also, set `builder` to `null` after a test is done
|
Commit message for review: Fixes: https://github.com/xamarin/xamarin-android/issues/7819
Our native runtime uses a cache of pointers to loaded managed assembly
images (essentially an array of the native `struct MonoImage` pointers)
which is pre-allocated at build time and placed in the native
`libxamarin-app.so` library.
While generating the cache, we also generate hashes for a number of
assembly name permutations (currently two per assembly: with and
without the extension). Only unique assembly names are considered when
generating the cache (it's possible to have duplicate names, because we
package more than one copy of some assemblies - those which are
architecture specific).
This algorithm had a bug which made it ignore culture prefix in
satellite assembly names (e.g. `en/MyAssembly.resources.dll`);
instead of several entries for each culture, we generated only two
entries (e.g. `MyAssembly.resources.dll` and `MyAssembly.resources`)
but we still counted each culture-prefixed assembly and stored that
number in `libxamarin-app.so` to be used at runtime to calculate number
of entries in the cache.
This made the array storing cached `MonoImage*` pointers to be smaller
than the number of actual assemblies in the APK times 2 and in some
cases we failed to look up pointer to some images and, as the result,
passed a `NULL` pointer to MonoVM which then caused a segmentation fault
trying to dereference the pointer:
F DEBUG : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x000000000000001c
F DEBUG : Cause: null pointer dereference
…
F DEBUG : #00 pc 00000000000bdad8 /data/app/~~7gCkYmQcKwTyS9NmxfgKxA==/com.companyname.bubby-PJIkJ0Lv0RiQhEVLeWi4wg==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (mono_class_get_checked+24) (BuildId: 80b786675a56824331f363bf29a2b54f454cf006)
Update the `<BuildApk/>` task to stop ignoring the culture prefix for
satellite assemblies in order to avoid the situation. Additionally,
since the previous assumption that MonoVM will validate all pointers
passed to its APIs turned out to be unwarranted, we now check more
carefully for `NULL` pointers when trying to obtain a native function
pointer from the MonoVM runtime.
Add a test for the issue, based on the
`MissingSatelliteAssemblyInLibrary()` packaging test which, when it
fails, will result in a `SIGABRT`:
D monodroid-assembly: assembly_store_open_from_bundles: looking for bundled name: 'System.Private.CoreLib' (hash 0x6b0ff375198b9c17)
F monodroid-assembly: Invalid assembly index 19, exceeds the maximum index of 11
F libc : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 17957 (semblyinlibrary), pid 17957 (semblyinlibrary)
Enhance the packaging test `MissingSatelliteAssemblyInLibrary()` by
adding more languages, to see if they're all packaged correctly. |
Comment on lines
+268
to
+269
| // We need to use the 'RelativePath' metadata, if found, because it will give us the correct path for satellite assemblies - with the culture in the path. | ||
| string? relativePath = assembly.GetMetadata ("RelativePath"); |
There was a problem hiding this comment.
Do we still need to change this to use %(DestinationSubPath)? I thought we saw that architecture-specific assemblies could also hit a similar issue?
There was a problem hiding this comment.
No, architecture specific assemblies are handled correctly (if they didn't, we'd see this issue much earlier). DestinationSubPath would only make matters worse - because I would have to know which is arch-specific assembly and which is a satellite assembly, to treat the prefix differently. RelativePath appears to be unambiguous for this purpose.
jonathanpeppers
approved these changes
Feb 27, 2023
dellis1972
approved these changes
Feb 28, 2023
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
Feb 28, 2023
* main: [monodroid] Properly process satellite assemblies (dotnet#7823) Bump to xamarin/java.interop/main@77800dda (dotnet#7824)
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
Feb 28, 2023
* main: [monodroid] Properly process satellite assemblies (dotnet#7823) Bump to xamarin/java.interop/main@77800dda (dotnet#7824)
|
Is there the same issue on iOS ? |
|
@softlion no, the code is specific to Xamarin.Android. If iOS shows similar issues then it's caused by something else there. |
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
Feb 28, 2023
* main: [Xamarin.Android.Build.Tasks] Remove support for mkbundle (dotnet#7772) [Xamarin.Android.Build.Tasks] `unable to open file as zip archive`? (dotnet#7759) [monodroid] Properly process satellite assemblies (dotnet#7823) Bump to xamarin/java.interop/main@77800dda (dotnet#7824)
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
Mar 6, 2023
* main: (22 commits) Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836) LEGO: Merge pull request 7852 [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832) [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848) [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837) [Mono.Android] Print type & member remapping info (dotnet#7844) [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785) LEGO: Merge pull request 7845 Localized file check-in by OneLocBuild Task (dotnet#7842) [ci] Use compliance stage template (dotnet#7818) [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840) Replace K4os.Hash.xxHash with System.IO.Hashing (dotnet#7831) $(AndroidPackVersionSuffix)=preview.3; net8 is 34.0.0-preview.3 (dotnet#7839) [Xamarin.Android.Build.Tasks] Remove support for mkbundle (dotnet#7772) [Xamarin.Android.Build.Tasks] `unable to open file as zip archive`? (dotnet#7759) [monodroid] Properly process satellite assemblies (dotnet#7823) Bump to xamarin/java.interop/main@77800dda (dotnet#7824) [ci] Use AZDO built-in parallelization strategy. (dotnet#7804) Bump to dotnet/installer@e3ab0b5 8.0.100-preview.2.23123.10 (dotnet#7813) [ci] Run nunit tests with stable .NET version (dotnet#7826) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #7819
Our native runtime uses a cache of pointers to loaded managed assembly
images (essentially an array of the native struct
MonoImagepointers)which is pre-allocated at build time and placed in the native
libxamarin-app.solibrary.While generating the cache, we also generate hashes for a number of
assembly name permutations (currently two per assembly - with and
without the extension). Only unique assembly names are considered when
generating the cache (it's possible to have duplicate names, because we
package more than one copy of some assemblies - those which are
architecture specific). This algorithm had a bug which made it ignore
culture prefix in satellite assembly names (e.g.
en/MyAssembly.resources.dll) and instead of several entries for eachculture, we generated only two entries for
MyAssembly.resources.dlland
MyAssembly.resourcesbut we still counted each culture-prefixedassembly and stored that number in
libxamarin-app.soto be used atruntime to calculate number of entries in the cache.
This made the array storing cached
MonoImage*pointers to be smallerthan the number of actual assemblies in the APK times 2 and in some
cases we failed to look up pointer to some images and, as the result,
passed a
NULLpointer to MonoVM which then caused a segmentation faulttrying to dereference the pointer.
Stop ignoring the culture prefix for satellite assemblies in order to
avoid the situation. Additionally, since the previous assumption that
MonoVM will validate all pointers passed to its APIs turned out to be
unwarranted, we now check more carefully for
NULLpointers when tryingto obtain a native function pointer from the MonoVM runtime.