Bug 49721 - Assembly binder uses wrong strongly named assembly when same assembly with different version exists in local folder
Summary: Assembly binder uses wrong strongly named assembly when same assembly with di...
Status: RESOLVED FIXED
Alias: None
Product: Runtime
Classification: Mono
Component: JIT (show other bugs)
Version: 4.8.0 (C9)
Hardware: PC Mac OS
: --- normal
Target Milestone: ---
Assignee: Aleksey Kliger
URL:
Depends on:
Blocks:
 
Reported: 2016-12-12 18:11 UTC by Marek Safar
Modified: 2017-04-13 18:28 UTC (History)
2 users (show)

See Also:
Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
test (7.23 KB, application/zip)
2016-12-12 18:11 UTC, Marek Safar
Details

Description Marek Safar 2016-12-12 18:11:41 UTC
Created attachment 18863 [details]
test

Using attached test do

mcs v1/lib.cs /target:library /keyfile:strong.snk
gacutil -i v1/lib.dll
Installed v1/lib.dll into the gac (/Users/marek/mono/lib/mono/gac)
mcs v2/lib.cs /target:library /keyfile:strong.snk
cp v2/lib.dll .
mcs app.cs -r:v1/lib.dll

mono app.exe

Mono: 
Unhandled Exception:
System.MissingMethodException: Method 'X.M1' not found.

.NET

No error
Comment 1 Aleksey Kliger 2017-02-28 23:18:12 UTC
(The first half of this is just notes on how I understand our loader, the second half is a proposed fix)

Why this is happening:

I did not know this, but apparently Mono and .NET Framework consider the GAC and the codebases/ApplicationBase directories in different orders:

  1. .NET Framework looks in the GAC first if the request is for a strong named assembly https://msdn.microsoft.com/en-us/library/yx7xezcf(v=vs.110).aspx If that doesn't work out it considers a collection of directories.
  2. The key function in Mono's implementation is mono_assembly_load_full_nosearch ()
     a. First we do various things to remap versions and apply bindings
     b. Then we call the "preload hook" of which the one that's installed in a vanilla Mono is mono_domain_assembly_preload () (from appdomain.c) which tries to load the requested assembly from the domain search path (which, if you do nothing at all, includes the dir where app.exe lives) and then MONO_PATH.
     c. If that didn't work out, we then try the GAC, then the base dir.

What I can't do:

I wish I could just change the preload hook to run after the GAC lookup (ie make Mono behave like .NET), but I'm told this will break customers who expect Mono's existing behavior (the earliest commit that I can find that codifies our order is from 2004, so).

What I'm doing:

I'm changing mono_assembly_load_from_full to take a predicate callback, and I'm threading the requested MonoAssemblyName from mono_domain_assembly_preload down to load_from_full.

The idea is that before we commit to an assembly (announced by calling mono_assembly_invoke_load_hook ()) we invoke the predicate and check that the candidate assembly we've got is good.  The check has to be in load_from_full because it's both where the new MonoAssembly is created and the place where invoke_load_hook is called - anywhere else is too late or too early.

load_from_full doesn't have the original MonoAssemblyName that we're looking for - it only has a MonoImage and a filepath - so at the very least we need to get the requested name down there, but since this is the second time I'm working in this function in less than a year, it seems better to have a bit more abstraction in case we have more reasons for not loading something.

This is enough both to keep roslyn building Mono's BCL and also get the above reproduction steps to work.  Please let me know if there are other scenarios I should try.
Comment 2 Aleksey Kliger 2017-03-16 23:26:26 UTC
Fixed on mono master https://github.com/mono/mono/commit/b8285f3c9951354be07f693dd5996790d5764e3a

Not backporting to mono 2017-02 branch at this time - too risky a change.

We should probably update the release notes once we make a mono release from master to document the new behavior.
Comment 3 Aleksey Kliger 2017-04-11 20:29:38 UTC
The version check, as committed on master is too strong when running with --runtime=mobile

Problem:
  The public key token on framework assemblies is different for the mobile profile.
Details:
  Running with --runtime=mobile will remap requests to load certain framework assemblies (see mono/metadata/assembly.c:94 "framework_assemblies" table) to different versions.  This is done because the mobile assemblies in Mono come from a variety of places (Silverlight, CoreFX, etc) and aren't the same as on the desktop.

  However the framework assemblies not only have different versions that on desktop, they also are signed with different public keys.

  So if some assembly references "System.Xml", Version=4.0.0.0, PublicKeyToken=XYZ, the runtime will remap that request to Version=2.0.5.0, PublicKeyToken=ABC.

  The code committed in Comment 2 assumed that ABC is always the silverlight public key token.  But it's not. For some framework assemblies it's Silverlight, but for others it might be CoreFX (or possibly ECMA, or something else, I'm not sure).

What I'm doing:

  I'm reverting the part of https://github.com/mono/mono/commit/b8285f3c9951354be07f693dd5996790d5764e3a that remaps the public key token to silverlight when --runtime=mobile is specified.  That's just wrong.

What's next:

  I need to either:

  1. Ignore the public key token if the request is for a framework assembly, and the version got remapped.
  2. Extend the remapping table to include public key tokens.  (There's already a promising looking remap_keys() function in assembly.c but it's actually for going the other way - it's for the situation where an assembly on a desktop mono runtime tries to load a Silverlight framework assembly, for example.  I'd either need to generalize this, or use another table.)
Comment 4 Marek Safar 2017-04-12 14:09:05 UTC
I think we can ignore public key token match if the request is for a framework assembly and the public key is one of the few known ones. Alternatively we could extend MonoRuntimeInfo::public_key_token to be an array of known/allowed values.

Note You need to log in before you can comment on or make changes to this bug.