Fix BinaryFormatter to properly deserialize objects with different ve… by krwq · Pull Request #20552 · dotnet/corefx

krwq · 2017-06-01T04:09:41Z

Fix BinaryFormatter to properly deserialize objects with different version when AssemblyFormat is set to Simple

needs clean up to match desktop more closely
once PR is merged replace last commit in Merge serialization changes to 2.0.0 #20530

…rsion when AssemblyFormat is set to Simple

morganbr · 2017-06-01T07:12:00Z

            {
                Assembly assm = null;
-                try
+                if (_isSimpleAssembly)


@stephentoub , we tried to make this logic match the semantics of https://referencesource.microsoft.com/#mscorlib/system/runtime/serialization/formatters/binary/binaryobjectreader.cs,84b78d568158e011 , but we didn't know the code well. Does this look right to you?

What prevents us from just using the desktop code as-is? Are we still missing some Assembly-related methods it depends on?

Desktop calls LoadWithPartialNameInternal which does not exist in Core. After that point Core diverges.

LoadWithPartialNameInternal calls nLoad(string...). This change does Assembly.Load(string) instead. In Desktop, Assembly.Load(string) eventually calls to nLoad with what seems to essentially be just the assembly name: so hopefully that is an equivalent substitution.

danmoseley · 2017-06-01T16:00:21Z

+                    }
+                    catch { }
+
+                    if (assm == null)


Per desktop this should also check assemblyName != null

is already checking here: https://github.com/dotnet/corefx/pull/20552/files#diff-b4b2fd9840862f03eae907efcd988a68R1011

danmoseley · 2017-06-01T16:01:13Z

+                    {
+                        assm = Assembly.Load(new AssemblyName(assemblyName));
+                    }
+                    catch { }


According to Desktop, the GetSimplyNamedTypeFromAssembly and FormatterServices.GetTypeFromAssembly calls should be inside the if/else here, not after the if/else.

it's the same behavior. First the assembly gets loaded, then it checks if is correctly loaded (!= null) and then it executes either GetSimplyNamedTypeFromAssembly or FormatterServices.GetTypeFromAssembly. This is just code cleanup.

I'm missing something. In Desktop, if bSimpleAssembly==true, and the load succeeds, it calls ObjectReader.GetSimplyNamedTypeFromAssembly. if bSimpleAssembly==false and the load succeeds, it calls FormatterServices.GetTypeFromAssembly. If either load fails it calls neither.

In this code, whatever the value of bSimplAssembly, if either load succeeds it calls GetSimplyNamedTypeFromAssembly otherwise it calls FormatterServices.GetTypeFromAssembly.

What am I missing?

In this code, whatever the value of bSimplAssembly, if either load succeeds it calls GetSimplyNamedTypeFromAssembly otherwise it calls FormatterServices.GetTypeFromAssembly.

No. If load succeeds and _isSimpleAssembly == true then it calls GetSimplyNamedTypeFromAssembly. If load succeeds and _isSimpleAssembly == false it calls FormatterServices.GetTypeFromAssembly. If load fails it calls nothing.

aagh -- the relevant if was collapsed in the diff, and the code looked completely valid collapsed.. thx!

ghost · 2017-06-01T16:09:39Z

+                if (_isSimpleAssembly)
                {
-                    assm = Assembly.Load(new AssemblyName(assemblyName));
+                    try


This is just an inlined copy of ResolveSimpleAssemblyName(). I would replace this with a call to that.

You're right - we tried to take minimum changes first but then realized that wasn't fully fixing a problem and had to in the end add the changes we wanted out of this PR - now we have all the pieces, I'll update this to match desktop

@krwq I'm already addressing the PR feedback

@atsushikan it's not the same. ResolveSimpleAssemblyName first tries to load the assembly with an AssemblyName type and if that fails it tries to load it with the name of the assembly as a string.

At this point we are first trying to resolve it by assemblyname (string) and if that fails by AssemblyName type. So vice versa.

At this point we are first trying to resolve it by assemblyname (string) and if that fails by AssemblyName type. So vice versa.

That wasn't 100% correct. First it tries to load it with the given assemblyname (string) and if that fails it creates an AssemblyName type and tries to load it with the name property of it. I don't think this is the same as ResolveSimpleAssemblyName.

No, it's the same logic - the only difference is that the first call is to Assembly.Load(String) rather than Load(AssemblyName). You could use the common function just by creating an AssemblyName out of the string "assemblyName" first (that's just what Assembly.Load(string) does internally so you're not adding overhead by doing this.)

ok then I'm fine changing it. Is it safe to create the AssemblyName outside of a try catch or should I also wrap one around it? I checked the sources and can't say for sure as it is calling an external method.

If the AssemblyName constructor throws, it's because the assembly name itself is badly formed. That's an exception that really shouldn't be swallowed, in my opinion.

I realize, though, that may be a "breaking change" so I won't complain too hard if you put it inside the try.

(On the similar note, catching every exception indiscriminately out of Assembly.Load() is also bound to be controversial - one more reason why I'd like to have just copy of that logic.)

ghost · 2017-06-01T16:11:06Z

        }
+
+        internal sealed class TopLevelAssemblyTypeResolver
+        {


I assume this came from the desktop given its retro style - I think a closure at the Type.GetType() callsite would be clearer these days.

yes it's a desktop copy.

At least, the "m_" should be changed to "_".

ghost · 2017-06-01T16:15:26Z

            catch (FileLoadException) { }
            catch (BadImageFormatException) { }
+
+            if((object)type == null)


Casting type to object before comparing to null isn't necessary. The == operator for MemberInfo ensures that null checks occur without invoking any members on potentially 3-rd party types (assuming such a thing could even make it to this point.)

Thanks, we saw Desktop code had this and though this might be changing logic around overriden equals - if this is equivalent let's remove it. cc: @morganbr

Fine by me.

ghost · 2017-06-01T16:17:48Z

+                if (assembly == null)
+                    assembly = m_topLevelAssembly;
+
+                return assembly.GetType(simpleTypeName, false, ignoreCase);


I prefer "throwOnError: false, ignoreCase: ignoreCase".

ghost · 2017-06-01T16:18:50Z

Other than these notes, lgtm.

danmoseley · 2017-06-01T17:10:26Z

            catch (BadImageFormatException) { }

-            if((object)type == null)
+            if(type == null)


nit:spacing after if

ViktorHofer · 2017-06-01T17:28:52Z

PTAL

ViktorHofer · 2017-06-01T17:29:14Z

@atsushikan Ah I see you just approved the changes. Thanks!

ViktorHofer · 2017-06-01T18:37:24Z

@dotnet-bot test Innerloop netfx Windows_NT Release x64 Build and Test

System.Transactions.Tests.CloneTxTests.Run(cloneType: RollbackDependent, isoLevel: Chaos, forcePromote: False, completeClone: False, expectedStatus: Aborted) [FAIL]

https://ci.dot.net/job/dotnet_corefx/job/master/job/netfx_windows_nt_release_prtest/965/

Fix BinaryFormatter to properly deserialize objects with different ve…

717ac61

…rsion when AssemblyFormat is set to Simple

dnfclas added the cla-already-signed label Jun 1, 2017

danmoseley requested review from morganbr and stephentoub June 1, 2017 04:10

morganbr reviewed Jun 1, 2017

View reviewed changes

morganbr approved these changes Jun 1, 2017

View reviewed changes

danmoseley requested a review from a user June 1, 2017 15:54

danmoseley reviewed Jun 1, 2017

View reviewed changes

ghost reviewed Jun 1, 2017

View reviewed changes

Addressing PR feedback

eb92bd5

danmoseley reviewed Jun 1, 2017

View reviewed changes

Use ResolveSimpleAssemblyName instead of inline code

bfb422d

ghost approved these changes Jun 1, 2017

View reviewed changes

stephentoub approved these changes Jun 1, 2017

View reviewed changes

danmoseley approved these changes Jun 1, 2017

View reviewed changes

danmoseley merged commit 19e4b90 into dotnet:master Jun 1, 2017

karelz modified the milestone: 2.1.0 Jun 2, 2017

Uh oh!

Conversation

krwq commented Jun 1, 2017 • edited by ViktorHofer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 1, 2017

Uh oh!

ViktorHofer commented Jun 1, 2017

Uh oh!

ViktorHofer commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

krwq commented Jun 1, 2017 •

edited by ViktorHofer

Loading

danmoseley Jun 1, 2017 •

edited

Loading

ViktorHofer Jun 1, 2017 •

edited

Loading

ViktorHofer Jun 1, 2017 •

edited

Loading

ViktorHofer commented Jun 1, 2017 •

edited

Loading