Loading denied assemblies via `Assembly.LoadFrom` can cause issues, eg. for VSMac #8726

radical · 2018-05-16T00:54:44Z

Usually, if an assembly depends on a known "bad"(windows specific implementation) assembly then the mono runtime can deny loading
it. But if the load is initiated explicitly like via Assembly.LoadFrom, then the assembly gets loaded (https://github.com/mono/mono/blob/2018-02/mono/metadata/image.c#L1363-L1365)
with a Loading problematic image ....

For example, take System.Runtime.InteropServices.RuntimeInformation.dll. It's RuntimeInformation.IsOSPlatform API's response
depends on what platform it was built was for. So, a windows specific version would return true even it is running on macOS. Normally,
this gets caught and denied loading. But if it is explicitly loaded via Assembly.LoadFrom, then that will break
any subsequent code that depends on that API!

This can show up in VSMac, if some addin loads the bad assembly before the good copy has been loaded into the app domain. Then the
subsequent use of msbuild APIs that depend on IsOSPlatform break:

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.TypeInitializationException: The type initializer for 'Microsoft.Build.Collections.MSBuildNameIgnoreCaseComparer' threw an exception. ---> System.EntryPointNotFoundException: GetSystemInfo
  at (wrapper managed-to-native) Microsoft.Build.Shared.NativeMethodsShared.GetSystemInfo(Microsoft.Build.Shared.NativeMethodsShared/SYSTEM_INFO&)

Here the windows specific code path runs on macOS, because of the IsOSPlatform's result!

Related: mono/monodevelop#4788

The text was updated successfully, but these errors were encountered:

radical · 2018-05-16T00:59:54Z

@lambdageek @kumpera @marek-safar @mhutch @akoeplinger

Therzok · 2018-05-16T09:48:22Z

Nice find!

marek-safar · 2018-05-16T12:14:08Z

@lambdageek did your context load changes fix the problem?

lambdageek · 2018-05-16T15:19:41Z

@marek-safar No: the load context work did nothing about "problematic images" - that's an entirely orthogonal concept.

The load context work changes the semantics of Assembly.LoadFrom so that it is now subject to binding redirection via app.config or policy config files, but I don't think that would help here - our System.Runtime.InteropServices.RuntimeInformation.dll has the same version and public key token as the .NET Framework one - binding redirection wouldn't help.

What's not clear to me is what should Assembly.LoadFrom do if you try to open a problematic image?

The simple answer is: fail - in that case we can just drop theif and only keep the else case:

mono/mono/metadata/image.c

Lines 1364 to 1370 in 3d8125e

    
           if (image->load_from_context) { 
        
           	mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_ASSEMBLY, "Loading problematic image %s", image->name); 
        
           } else { 
        
           	mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_ASSEMBLY, "Denying load of problematic image %s", image->name); 
        
           	*status = MONO_IMAGE_IMAGE_INVALID; 
        
           	goto invalid_image; 
        
           }

But that will break all callers of Assembly.LoadFrom - like whatever Monodevelop is doing in the bug that @radical mentioned.

A better answer might be - "Assembly.LoadFrom should silently redirect to find our preferred version of the problematic image". That's... doable but messy (because the loader is a mess) I wouldn't want to backport that to an imminent release.

So let me know what it should do.

mhutch · 2018-05-16T16:23:31Z

Rewinding and adding full context :)

NuGet packagers assume that ".net framework" means "windows", so folks end up with windows-specific assemblies (often transitive dependencies) copied into their output folder without even realizing it, which then breaks on Windows. This is an issue with several very common system.* packages, so we have the Load deny mechanism for these specifically.

The problem in this case is that VSMac uses LoadFrom to load extensions' assemblies, and the deny mechanism only works with Load, not LoadFrom.

I think silently redirecting LoadFrom to load the good variant when an image is denied is entirely reasonable and consistent. This isn't totally general, so scope is limited. It's just swapping out specific known bad assemblies for good replacements that are bundled with Mono.

One could argue that the extensions shouldn't be shipping these assemblies. For the assemblies that are included in MD core that's definitely true, and perhaps VSMac could add special handling to avoid loading them from extensions. However, for assemblies that are not bundled in MD core, the extension needs to bundle them to work on .NET Framework.

I understand the concern about backporting the patch. As a stopgap, we could implement the same blocklist in Mono.Addins so MD doesn't get broken by extensions. We've seen this breakage in the wild with the stable 7.5 release. /cc @slluis.

The "problematic images" are Windows-specific assemblies that are often included with applications that do not work on Mono, but for which Mono has its own implementation. Previously (9ec8ec5) we changed mono to deny loading problematic images using Assembly.Load or when one assembly references another and we find a problematic image using the usual assembly search. For Assembly.LoadFrom, we used to emit a warning and then load the assembly anyway. In this commit, we change the behavior: If Assembly.LoadFrom tries to open a problematic image, we will instead probe for an assembly of that name in the default context. That should eventually end up opening Mono's version of that assembly. Example: Suppose a problematic System.Runtime.InteropServices.RuntimeInformation.dll is in the application bin path: /home/user/myapp/bin 1. User code does (for whatever reason) Assembly.LoadFrom ("/home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll") 2. We find the image and try to open it; add it to the images absfpath to image hash; note that there are no binding redirects; but that it is a problematic image. We extract the assembly name ("System.Runtime.InteropServices.RuntimeInformation", version x.y.z.w and public key token 123456789a) from the problematic image. 3. We probe for "System.Runtime.InteropServices.RuntimeInformation version x.y.z.w public key token 123456789a" in the default context. 4. We find /home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll in the application bin path; we try to open it, we see it's already in the images hash, but it's a problematic image, and we're not in a loadfrom context (we re-probed with default!) so we keep probing 5. We find <prefix>/lib/mono/4.5/Facaded/System.Runtime.InteropServices.RuntimeInformation.dll, it's not a problematic image, so we open that and return it to (3) 6. We return to (2) and close the problematic image and return the good one instead. 7. Assembly.LoadFrom returns with the good image. Fixes mono#8726

marek-safar · 2018-05-17T07:26:01Z

@lambdageek I certainly prefer consistent behaviour in this case and we should use same logic as we do when loading the assembly using other API. That leaves use with only reflection context APIs behaving differently but that sounds reasonable to me.

I'd like to see this in 2018-04 because we are still testing the release and we can take it out if necessary. I agree that backporting to earlier versions (15.8 and before) might be problematic but we'd have to find a different workaround in that case.

The "problematic images" are Windows-specific assemblies that are often included with applications that do not work on Mono, but for which Mono has its own implementation. Previously (9ec8ec5) we changed mono to deny loading problematic images using Assembly.Load or when one assembly references another and we find a problematic image using the usual assembly search. For Assembly.LoadFrom, we used to emit a warning and then load the assembly anyway. In this commit, we change the behavior: If Assembly.LoadFrom tries to open a problematic image, we will instead probe for an assembly of that name in the default context. That should eventually end up opening Mono's version of that assembly. Example: Suppose a problematic System.Runtime.InteropServices.RuntimeInformation.dll is in the application bin path: /home/user/myapp/bin 1. User code does (for whatever reason) Assembly.LoadFrom ("/home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll") 2. We find the image and try to open it; add it to the images absfpath to image hash; note that there are no binding redirects; but that it is a problematic image. We extract the assembly name ("System.Runtime.InteropServices.RuntimeInformation", version x.y.z.w and public key token 123456789a) from the problematic image. 3. We probe for "System.Runtime.InteropServices.RuntimeInformation version x.y.z.w public key token 123456789a" in the default context. 4. We find /home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll in the application bin path; we try to open it, we see it's already in the images hash, but it's a problematic image, and we're not in a loadfrom context (we re-probed with default!) so we keep probing 5. We find <prefix>/lib/mono/4.5/Facaded/System.Runtime.InteropServices.RuntimeInformation.dll, it's not a problematic image, so we open that and return it to (3) 6. We return to (2) and close the problematic image and return the good one instead. 7. Assembly.LoadFrom returns with the good image. Fixes mono#8726

The "problematic images" are Windows-specific assemblies that are often included with applications that do not work on Mono, but for which Mono has its own implementation. Previously (9ec8ec5) we changed mono to deny loading problematic images using Assembly.Load or when one assembly references another and we find a problematic image using the usual assembly search. For Assembly.LoadFrom, we used to emit a warning and then load the assembly anyway. In this commit, we change the behavior: If Assembly.LoadFrom tries to open a problematic image, we will instead probe for an assembly of that name in the default context. That should eventually end up opening Mono's version of that assembly. Example: Suppose a problematic System.Runtime.InteropServices.RuntimeInformation.dll is in the application bin path: /home/user/myapp/bin 1. User code does (for whatever reason) Assembly.LoadFrom ("/home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll") 2. We find the image and try to open it; add it to the images absfpath to image hash; note that there are no binding redirects; but that it is a problematic image. We extract the assembly name ("System.Runtime.InteropServices.RuntimeInformation", version x.y.z.w and public key token 123456789a) from the problematic image. 3. We probe for "System.Runtime.InteropServices.RuntimeInformation version x.y.z.w public key token 123456789a" in the default context. 4. We find /home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll in the application bin path; we try to open it, we see it's already in the images hash, but it's a problematic image, and we're not in a loadfrom context (we re-probed with default!) so we keep probing 5. We find <prefix>/lib/mono/4.5/Facaded/System.Runtime.InteropServices.RuntimeInformation.dll, it's not a problematic image, so we open that and return it to (3) 6. We return to (2) and close the problematic image and return the good one instead. 7. Assembly.LoadFrom returns with the good image. Fixes #8726

marek-safar · 2018-05-23T10:16:00Z

@lambdageek how confident are you to landing this to 2018-02?

The "problematic images" are Windows-specific assemblies that are often included with applications that do not work on Mono, but for which Mono has its own implementation. Previously (9ec8ec5) we changed mono to deny loading problematic images using Assembly.Load or when one assembly references another and we find a problematic image using the usual assembly search. For Assembly.LoadFrom, we used to emit a warning and then load the assembly anyway. In this commit, we change the behavior: If Assembly.LoadFrom tries to open a problematic image, we will instead probe for an assembly of that name in the default context. That should eventually end up opening Mono's version of that assembly. Example: Suppose a problematic System.Runtime.InteropServices.RuntimeInformation.dll is in the application bin path: /home/user/myapp/bin 1. User code does (for whatever reason) Assembly.LoadFrom ("/home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll") 2. We find the image and try to open it; add it to the images absfpath to image hash; note that there are no binding redirects; but that it is a problematic image. We extract the assembly name ("System.Runtime.InteropServices.RuntimeInformation", version x.y.z.w and public key token 123456789a) from the problematic image. 3. We probe for "System.Runtime.InteropServices.RuntimeInformation version x.y.z.w public key token 123456789a" in the default context. 4. We find /home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll in the application bin path; we try to open it, we see it's already in the images hash, but it's a problematic image, and we're not in a loadfrom context (we re-probed with default!) so we keep probing 5. We find <prefix>/lib/mono/4.5/Facaded/System.Runtime.InteropServices.RuntimeInformation.dll, it's not a problematic image, so we open that and return it to (3) 6. We return to (2) and close the problematic image and return the good one instead. 7. Assembly.LoadFrom returns with the good image. Fixes mono#8726

lambdageek · 2018-05-23T15:01:43Z

@marek-safar I think it's probably worth backporting. It's not too disruptive. #8845

lambdageek · 2018-06-15T14:09:24Z

We had to revert the fix for this issue on 2018-06 and 2018-04. It's fixed on master (need at least 60f06e6 to both fix this issue and also not break MSBuild).

mhutch · 2018-06-15T17:39:48Z

@lambageek is that a temporary revert or do we need to think about alternative workarounds for MD running on those releases?

/cc @slluis

lambdageek · 2018-06-15T17:55:10Z

@mhutch The main motivation for the reverts was to unblock XI and XA moving to 2018-04 and to re-enable nightly CI builds of the MDK. The corrected fix for 2018-06 is in PR #9170. ~~I'm going to do the 2018-04 updated PR imminently.~~ The corrected 2018-04 fix is in #9171

lambdageek · 2018-06-19T20:42:51Z

Ok, this should be fixed (again) on 2018-04 (commit f22c8b7) and 2018-06 (commit 9ec4d30).

Closing.

Therzok · 2018-06-26T10:07:24Z

What about 2018-02 (5.12 series)?

lambdageek · 2018-06-28T15:00:09Z

We can do a backport to 2018-02, but it is unlikely to make it into any stable release.

slluis · 2018-06-28T15:16:49Z

@lambdageek why not? 2018-02 is part of the 15.8 release.

lambdageek · 2018-06-28T15:57:21Z

@slluis @Therzok Was this a regression?

Backport for 2018-02 is at #9367

/cc @luhenry

Therzok · 2018-07-04T21:12:03Z

Our MSBuild builder just started crashing, so yes, it was a regression.

The "problematic images" are Windows-specific assemblies that are often included with applications that do not work on Mono, but for which Mono has its own implementation. Previously (mono/mono@9ec8ec5) we changed mono to deny loading problematic images using Assembly.Load or when one assembly references another and we find a problematic image using the usual assembly search. For Assembly.LoadFrom, we used to emit a warning and then load the assembly anyway. In this commit, we change the behavior: If Assembly.LoadFrom tries to open a problematic image, we will instead probe for an assembly of that name in the default context. That should eventually end up opening Mono's version of that assembly. Example: Suppose a problematic System.Runtime.InteropServices.RuntimeInformation.dll is in the application bin path: /home/user/myapp/bin 1. User code does (for whatever reason) Assembly.LoadFrom ("/home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll") 2. We find the image and try to open it; add it to the images absfpath to image hash; note that there are no binding redirects; but that it is a problematic image. We extract the assembly name ("System.Runtime.InteropServices.RuntimeInformation", version x.y.z.w and public key token 123456789a) from the problematic image. 3. We probe for "System.Runtime.InteropServices.RuntimeInformation version x.y.z.w public key token 123456789a" in the default context. 4. We find /home/user/myapp/bin/System.Runtime.InteropServices.RuntimeInformation.dll in the application bin path; we try to open it, we see it's already in the images hash, but it's a problematic image, and we're not in a loadfrom context (we re-probed with default!) so we keep probing 5. We find <prefix>/lib/mono/4.5/Facaded/System.Runtime.InteropServices.RuntimeInformation.dll, it's not a problematic image, so we open that and return it to (3) 6. We return to (2) and close the problematic image and return the good one instead. 7. Assembly.LoadFrom returns with the good image. Fixes mono/mono#8726 Commit migrated from mono/mono@9ad5255

radical assigned kumpera and lambdageek and unassigned kumpera and lambdageek May 16, 2018

marek-safar added the area-Runtime: Loader Any assembly loading/binding issue label May 16, 2018

marek-safar added this to the 2018-04 (5.14.xx) milestone May 16, 2018

mhutch mentioned this issue May 16, 2018

Add blocklist for known bad assemblies to prevent extensions breaking things mono/monodevelop#4823

Open

lambdageek self-assigned this May 16, 2018

lambdageek mentioned this issue May 17, 2018

[loader] LoadFrom of problematic images should reprobe #8737

Merged

Therzok mentioned this issue May 17, 2018

SourceEditor is not loading types, no intellisense with a log growing with a lot of errors mono/monodevelop#4771

Closed

lambdageek mentioned this issue May 17, 2018

[2018-04][loader] LoadFrom of problematic images should reprobe #8749

Merged

radical mentioned this issue May 21, 2018

VS for Mac freezes unexpectedly mono/monodevelop#4788

Closed

marek-safar modified the milestones: 2018-04 (5.14.xx), 2018-02 (5.12.xx) May 23, 2018

marek-safar closed this as completed in #8737 May 23, 2018

lambdageek mentioned this issue May 23, 2018

[2018-02][loader] LoadFrom of problematic images should reprobe #8845

Closed

kumpera mentioned this issue Jun 13, 2018

Revert "[loader] LoadFrom of problematic images should reprobe" #9084

Closed

lambdageek reopened this Jun 15, 2018

lambdageek mentioned this issue Jun 15, 2018

[2018-06][loader] LoadFrom of problematic images should reprobe (correctly) #9170

Merged

lambdageek mentioned this issue Jun 15, 2018

[2018-04][loader] LoadFrom of problematic images should reprobe (correctly) #9171

Merged

lambdageek closed this as completed Jun 19, 2018

lambdageek mentioned this issue Jun 28, 2018

[2018-02][loader] LoadFrom of problematic images should reprobe (correctly) #9367

Merged

marek-safar modified the milestones: 2018-04 (5.14.xx), 2018-02 (5.12.xx) Jul 9, 2018

directhex mentioned this issue Sep 20, 2018

Bump to mono:2018-08 xamarin/xamarin-macios#4596

Merged

directhex mentioned this issue Oct 1, 2018

Bump to mono:2018-06 xamarin/xamarin-macios#4277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading denied assemblies via `Assembly.LoadFrom` can cause issues, eg. for VSMac #8726

Loading denied assemblies via `Assembly.LoadFrom` can cause issues, eg. for VSMac #8726

radical commented May 16, 2018

radical commented May 16, 2018

Therzok commented May 16, 2018

marek-safar commented May 16, 2018

lambdageek commented May 16, 2018

mhutch commented May 16, 2018

marek-safar commented May 17, 2018

marek-safar commented May 23, 2018

lambdageek commented May 23, 2018

lambdageek commented Jun 15, 2018

mhutch commented Jun 15, 2018

lambdageek commented Jun 15, 2018 •

edited

Loading

lambdageek commented Jun 19, 2018

Therzok commented Jun 26, 2018

lambdageek commented Jun 28, 2018

slluis commented Jun 28, 2018

lambdageek commented Jun 28, 2018

Therzok commented Jul 4, 2018

Loading denied assemblies via Assembly.LoadFrom can cause issues, eg. for VSMac #8726

Loading denied assemblies via Assembly.LoadFrom can cause issues, eg. for VSMac #8726

Comments

radical commented May 16, 2018

radical commented May 16, 2018

Therzok commented May 16, 2018

marek-safar commented May 16, 2018

lambdageek commented May 16, 2018

mhutch commented May 16, 2018

marek-safar commented May 17, 2018

marek-safar commented May 23, 2018

lambdageek commented May 23, 2018

lambdageek commented Jun 15, 2018

mhutch commented Jun 15, 2018

lambdageek commented Jun 15, 2018 • edited Loading

lambdageek commented Jun 19, 2018

Therzok commented Jun 26, 2018

lambdageek commented Jun 28, 2018

slluis commented Jun 28, 2018

lambdageek commented Jun 28, 2018

Therzok commented Jul 4, 2018

Loading denied assemblies via `Assembly.LoadFrom` can cause issues, eg. for VSMac #8726

Loading denied assemblies via `Assembly.LoadFrom` can cause issues, eg. for VSMac #8726

lambdageek commented Jun 15, 2018 •

edited

Loading