Validate modules that are part of a version bubble#31822
Validate modules that are part of a version bubble#31822MichalStrehovsky wants to merge 2 commits into
Conversation
This adds validation that ensures the modules that we compiled as part of the large version bubble are the same modules that we see at runtime. If a different module is detected at runtime a FileLoadException will be thrown. This includes the change to the runtime, change to the crossgen2 compiler, and change to r2rdump.
|
Could you please add a test(s) for this? I know such tests are not pretty (like the current R2R versioning test), but I think it is important to have at least some coverage for it. |
|
|
||
| // Check our own assembly first | ||
| GetAppDomain()->CheckForMismatchedNativeImages(&spec, &pVersionInfo->sourceAssembly.mvid); | ||
| GetLoaderAllocator()->CheckForMismatchedNativeImages(&spec, &pVersionInfo->sourceAssembly.mvid); |
There was a problem hiding this comment.
I do not think that loader allocator is the right place to attach this to. If you have multiple (non-unlodable) AssemblyLoadContexts, all of them will share the one loader allocator, but it should be possible to load different versions of assemblies into each one.
In other words, loader allocator is lifetime unit; but this should be attached to assembly binding unit.
I know that the assembly loader is a mess and so the best way to do this may not be obvious.
There was a problem hiding this comment.
@davidwrighton could you help me find a home for this? I kicked it out of AppDomain based on your feedback, but it can't live in LoaderAllocator either.
There was a problem hiding this comment.
I have looked around:
- The unmanaged equivalent of
AssemblyLoadContextisICLRPrivBinderinterface ICLRPrivBinderinterface is implemented by several actual types- There does not seem to be a good way to share stuff between these actual types
I think we need to:
- Create unmanaged
class AssemblyLoadContext - Put it between the managed
AssemblyLoadContextandICLRPrivBinder(e.g. the unmanagedAssemblyLoadContextis going to haveICLRPrivBinder* m_pCLRBinderfield) - Attach this structure to it
@elinor-fung @vitek-karas Thoughts?
There was a problem hiding this comment.
I think that would be reasonable. However, the assembly loader is such a tangle of indirection that I'm not sure how nice adding another layer would be. Potential related (existing) weirdness: the managed default AssemblyLoadContext class is created on demand, so default binder (and corresponding unmanaged AssemblyLoadContext) would exist without it, and the WinRT binder has no dedicated managed ALC representation
There is also the ApplicationContext instance (on each binder implementation). It has things like the cache of loaded managed assemblies, so I think it is kind of representative of the ALC as well. It might be fitting to have the mapping of the native image names/mvids there? It is currently only exposed on the concrete implementations (not ICLRPrivBinder) though.
The CLRPrivBinderCoreCLR and CLRPrivBinderAssemblyLoadContext implementations do have a lot of duplication (a bit less so for CLRPrivBinderWinRT). Adding a shared base for them might be nice in general.
There was a problem hiding this comment.
If I remember correctly we already discussed the fact that the interfaces (IClrPrivBinder) are not necessary and should be mostly removed (especially the fact that they are defined the "COM" way). I think that the solution here could be that we replace the IClrPrivBinder with an abstract base class, which can then store whatever additional state we need. It would also allow for an easier way to share code between the binders (as @elinor-fung notes, there's quite a bit of overlap in some cases).
There was a problem hiding this comment.
I have done some minimal refactoring to introduce AssemblyLoadContext base class in: #32097
| publicKeyOrToken: metadataBuilder.GetOrAddBlob(publicKeyOrToken), | ||
| publicKeyOrToken: publicKeyOrToken == null ? default : metadataBuilder.GetOrAddBlob(publicKeyOrToken), | ||
| flags: assemblyFlags, | ||
| hashValue: default(BlobHandle) /* TODO */); |
There was a problem hiding this comment.
Why don't we just use the hashValue here to store the MVID instead of a new node type?
There was a problem hiding this comment.
Unless of course we have plans to use hashValue for something else, but the list of assemblies written in this MD blob seem to only be used for the version bubble stuff.
There was a problem hiding this comment.
This table only has references to assemblies that don't exist in the original AssemblyRef table. There's a complex scheme of how to make sense of it.
We can't modify the original AssemblyRef table because it's visible to user code. We would need an alternative storage mechanism for those entries anyway - people do weird stuff with user visible parts of these things - e.g. I've seen an app that used the assembly's public key to encrypt network traffic 😱).
There was a problem hiding this comment.
Ok Thanks Michal for the clarification
|
When we update this PR, we need to add #35231 (comment) part 4 to this PR's code. |
|
@MichalStrehovsky, are you still working on this? Hasn't been updated in 8 months. Thanks. |
ghost
locked as resolved and limited conversation to collaborators
8 participants
This adds validation that ensures the modules that we compiled as part of the large version bubble are the same modules that we see at runtime. If a different module is detected at runtime a FileLoadException will be thrown.
This includes the change to the runtime, change to the crossgen2 compiler, and change to r2rdump.
Cc @dotnet/crossgen-contrib