Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization error in nightly build #10945

Closed
jaredpar opened this issue Oct 2, 2018 · 10 comments · Fixed by #11134

Comments

@jaredpar
Copy link
Contributor

@jaredpar jaredpar commented Oct 2, 2018

Steps to Reproduce

This was found attempting to get the C# semantic tests running on Mono

  1. Clone https://github.com/jaredpar/roslyn
  2. Check out the branch repro/mono-serialize
  3. Make sure mono is on your path
  4. Run ./build.sh --restore --build
  5. Run ./build/scripts/test.sh Debug mono Microsoft.CodeAnalysis.CSharp.Symbo

Actual Behavior

This will run the Symbol unit tests only and eventually Mono will throw an exception trying to call ITestCase.Traits across AppDomain instances. The error is a NullReferenceException during serialization

Expected Behavior

The xunit process runs to completion

On which platforms did you notice this

This was discovered on Ubuntu 18.04. I'm using the latest nightly mono as described by the Download page

https://www.mono-project.com/download/nightly/#download-lin

Mono JIT compiler version 5.19.0.290 (tarball Tue Sep  4 10:59:48 UTC 2018)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
	TLS:           __thread
	SIGSEGV:       altstack
	Notifications: epoll
	Architecture:  amd64
	Disabled:      none
	Misc:          softdebug 
	Interpreter:   yes
	LLVM:          yes(4)
	Suspend:       hybrid
	GC:            sgen (concurrent by default)

Stacktrace and full details

    [FATAL ERROR] System.NullReferenceException
      System.NullReferenceException : Object reference not set to an instance of an object
      Stack Trace:
        
        
        Server stack trace: 
          at System.Runtime.Serialization.FormatterServices.GetParentTypes (System.RuntimeType parentType, System.RuntimeType[]& parentTypes, System.Int32& parentTypeCount) [0x0001f] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.FormatterServices.InternalGetSerializableMembers (System.RuntimeType type) [0x00089] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.FormatterServices+<>c__DisplayClass9_0.<GetSerializableMembers>b__0 (System.Runtime.Serialization.MemberHolder _) [0x00000] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Collections.Concurrent.ConcurrentDictionary`2[TKey,TValue].GetOrAdd (TKey key, System.Func`2[T,TResult] valueFactory) [0x00034] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.FormatterServices.GetSerializableMembers (System.Type type, System.Runtime.Serialization.StreamingContext context) [0x0005e] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.InitMemberInfo () [0x0003d] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.InitSerialize (System.Object obj, System.Runtime.Serialization.ISurrogateSelector surrogateSelector, System.Runtime.Serialization.StreamingContext context, System.Runtime.Serialization.Formatters.Binary.SerObjectInfoInit serObjectInfoInit, System.Runtime.Serialization.IFormatterConverter converter, System.Runtime.Serialization.Formatters.Binary.ObjectWriter objectWriter, System.Runtime.Serialization.SerializationBinder binder) [0x00158] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.Serialize (System.Object obj, System.Runtime.Serialization.ISurrogateSelector surrogateSelector, System.Runtime.Serialization.StreamingContext context, System.Runtime.Serialization.Formatters.Binary.SerObjectInfoInit serObjectInfoInit, System.Runtime.Serialization.IFormatterConverter converter, System.Runtime.Serialization.Formatters.Binary.ObjectWriter objectWriter, System.Runtime.Serialization.SerializationBinder binder) [0x00006] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.ObjectWriter.Write (System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo objectInfo, System.Runtime.Serialization.Formatters.Binary.NameInfo memberNameInfo, System.Runtime.Serialization.Formatters.Binary.NameInfo typeNameInfo) [0x00101] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.ObjectWriter.Serialize (System.Object graph, System.Runtime.Remoting.Messaging.Header[] inHeaders, System.Runtime.Serialization.Formatters.Binary.__BinaryWriter serWriter, System.Boolean fCheck) [0x001d3] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize (System.IO.Stream serializationStream, System.Object graph, System.Runtime.Remoting.Messaging.Header[] headers, System.Boolean fCheck) [0x0006e] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize (System.IO.Stream serializationStream, System.Object graph, System.Runtime.Remoting.Messaging.Header[] headers) [0x00000] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize (System.IO.Stream serializationStream, System.Object graph) [0x00000] in <5a872f306a874e34bfe4796f739b7324>:0 
          at System.Runtime.Remoting.RemotingServices.SerializeCallData (System.Object obj) [0x00053] in <5a872f306a874e34bfe4796f739b7324>:0 
          at (wrapper xdomain-dispatch) Xunit.Sdk.TestMethodTestCase.get_Traits(object,byte[]&,byte[]&)
        
        Exception rethrown at [0]: 
          at (wrapper xdomain-invoke) Xunit.Sdk.TestMethodTestCase.get_Traits()
          at MessageSinkMessageExtensions.Dispatch[TMessage] (Xunit.Abstractions.IMessageSinkMessage message, System.Collections.Generic.HashSet`1[T] messageTypes, Xunit.MessageHandler`1[TMessage] callback) [0x0001a] in <7906e2ced2224c19a10a02057b1365de>:0 
          at (wrapper remoting-invoke-with-check) Xunit.MessageSinkAdapter.OnMessageWithTypes(Xunit.Abstractions.IMessageSinkMessage,System.Collections.Generic.HashSet`1<string>)
          at (wrapper xdomain-dispatch) Xunit.MessageSinkAdapter.OnMessageWithTypes(object,byte[]&,byte[]&)
        
        Exception rethrown at [1]: 
          at (wrapper xdomain-invoke) Xunit.MessageSinkAdapter.OnMessageWithTypes(Xunit.Abstractions.IMessageSinkMessage,System.Collections.Generic.HashSet`1<string>)

@jaredpar

This comment has been minimized.

Copy link
Contributor Author

@jaredpar jaredpar commented Oct 2, 2018

Note: this appears to be a regression between the stable and nightly Mono feeds. These tests all run fine in stable but fail as soon as I move to nightly.

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Oct 3, 2018

@kg could you look into this one

@kg

This comment has been minimized.

Copy link
Member

@kg kg commented Oct 3, 2018

Started looking at it earlier, will try to come up with a fix

@kg

This comment has been minimized.

Copy link
Member

@kg kg commented Oct 6, 2018

I spent most of the week on this and am completely stumped:

  • BinaryFormatter is failing to serialize a dict<string, list> containing one key/value pair, where the value is a list with one string
  • It fails for different string values
  • It is almost 100% reproducible but not 100%
  • Threading is not required for it to fail, and there is a lock around the only bits of shared state it uses (the formatter and its surrogate selector)
  • Replicating all the relevant logic in a reduced test case - either by hand or via remoting - does not reproduce the issue
  • The code responsible for serializing the data is pretty simple. I've ruled out every failure theory I can come up with
  • Deserializing the bundle immediately after serializing it (in the same domain) fails, just like in the destination domain, so it isn't a problem specific to crossing domains
  • The bad bundle fails to deserialize on windows .net framework with the same* error as it does for us w/referencesource

My only guess is that this is some sort of corruption issue related to the fact that the strings are coming directly out of reflection metadata (to be specific, custom attributes that specify traits for xunit test cases). But the strings look totally valid, and we're not getting any sort of heap corruption crashes or garbage data when examining the managed objects... The trait data looks correct, also.

We haven't touched any of the involved formatter code in ages as far as I can tell, so if we broke this it's somehow broken by changes in some related system. I'll try to bisect it but it's tricky since the process of building and running the tests is 15-30 minutes per go.

@kg

This comment has been minimized.

Copy link
Member

@kg kg commented Oct 12, 2018

This appears to have been regressed by 9dc6c34 from my bisect.

@kg

This comment has been minimized.

Copy link
Member

@kg kg commented Oct 12, 2018

I did some tracing in the custom modifier comparisons:

    Microsoft.CodeAnalysis.CSharp.UnitTests.Symbols.GenericConstraintTests.ImplicitImplementation [SKIP]
      Test supported only on CLR
has_cmods mismatch: 0 1
cmod rejected types for equality that were otherwise equal:
'System.Int32'
'System.Int32'
  cmods[0] = 'IsLong'
cmod count mismatch: 2 1
cmod rejected types for equality that were otherwise equal:
'System.Int32'
  cmods[0] = 'IsConst'
  cmods[1] = 'IsLong'
'System.Int32'
  cmods[0] = 'IsConst'
cmod count mismatch: 2 1
cmod rejected types for equality that were otherwise equal:
'System.Int32'
  cmods[0] = 'IsConst'
  cmods[1] = 'IsLong'
'System.Int32'
  cmods[0] = 'IsConst'
has_cmods mismatch: 1 0
cmod rejected types for equality that were otherwise equal:
'System.Int32'
  cmods[0] = 'IsLong'
'System.Int32'
has_cmods mismatch: 1 0
cmod rejected types for equality that were otherwise equal:
'System.Int32'
  cmods[0] = 'IsLong'
'System.Int32'
has_cmods mismatch: 1 0
cmod rejected types for equality that were otherwise equal:
'System.Int32'
  cmods[0] = 'IsLong'
'System.Int32'
has_cmods mismatch: 1 0
cmod rejected types for equality that were otherwise equal:
'System.Int32'
  cmods[0] = 'IsLong'

In earlier testing they were actually System.Int32&. I don't know how to find out which test this is happening in because the unit runner appears to have no way to get it to print test names. Something about these equality mismatches is thoroughly breaking the reflection or serialization infrastructure so it never works right again.

marek-safar added a commit that referenced this issue Oct 26, 2018
Fixes #10945
Reverts fix for #6936

#10945 appears to have been introduced by 9dc6c34. Unfortunately it is not possible to revert the commit at this point using git revert, but manually disabling the equality change fixes BinaryFormatter. This should be sufficient to get a lot of the broken roslyn tests working again.
@kg

This comment has been minimized.

Copy link
Member

@kg kg commented Oct 26, 2018

@jaredpar, if you use latest mono those hangs should be gone now. I suspect we may have regressed some other cmod related tests, though I did not see any failures when I ran the test suite locally.

jaredpar added a commit to jaredpar/roslyn that referenced this issue Jan 25, 2019
The serialization bug has popped up again. Disabling to get CI green
while Mono takes a look.

mono/mono#10945
@jaredpar

This comment has been minimized.

Copy link
Contributor Author

@jaredpar jaredpar commented Jan 28, 2019

Shouldn't this bug be Open given this is still reproducing?

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Jan 28, 2019

Yep, reopening

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Feb 18, 2019

Closing this again as it was confirmed as misconfiguration on Roslyn side

marek-safar added a commit that referenced this issue Mar 14, 2019
…3320)

This all stems from ECMA 335, 7.1.1:

> The CLI itself shall treat required and optional modifiers in the same
> manner. Two signatures that differ only by the addition of a custom
> modifier (required or optional) shall not be considered to match. Custom
> modifiers have no other effect on the operation of the VES.

This rule means that when comparing signatures, we have to treat two types that
differ only in their custom modifiers as distinct. In particular, we have to do
this when comparing signatures in order to fill in the vtable for a class.  And
moreover, we have to concatenate custom modifiers when doing generic
instantiation.  If a method in a generic type has signature `T modopt(IsConst)
Method()` and we instantiate T with `int32 modopt(IsLong)`, we are expected to
produce the signature `int32 modopt(IsConst) modopt(IsLong) Method()` (with the
custom modifiers on `int32` in that order).

So this PR implements the following operations:
1. custom modifier appending during `mono_metadata_type_dup_with_cmods`
2. custom modifier-respecting comparison in `do_mono_metadata_type_equal`

To do this, we actually have to introduce a second representation of custom
modifiers: in the normal representation, all the cmod type specs come from a
single image.  Whereas now, due to inflation of signatures, we can have a mix
of modifiers from different images.  So in the MonoAggregateModContainer
representation, we pair each typespec token with the image in which it is to be
resolved.

We keep the old representation (a single image to resolve the entire set of
typespec tokens) because it's natural when you're just reading from a single
image.  Also this representation is part of the embedding API.

---

### But that's not all ###


It is possible for generic parameters of a generic class to appear in the
_custom modifiers_ of a type in a method signature.  For example:
```
    T modopt(Nullable`1<T>) Method ()
```

In this case, when we inflate this signature, we need to inflate not only the
return type, but also the modifiers on a return type.

That means our old aggregate representation (where each modifier is a pair of a
MonoImage and a typespec token) is inadequate: we can inflate a signature and
get a custom modifier that doesn't appear as a typespec in any table of any
existing image.

This is the same situation as generic instances: as a program runs, it creates
generic instances that are not part of the source code.  So we solve it the
same way as with generic instances.

Our new `MonoAggregateModContainer` representation is a pointer into memory owned
by a MonoImageSet that contains an entire MonoType for each custom modifier.

The invariant is that the aggregate modifier container is allocated from a
mempool of a `MonoImageSet` that includes the images of all the modifiers.

In general, when inflating, we first create a provisional candidate
MonoAggregateModContainer on the stack and then look it up the image set that
should own it and then look for a "canonical" mod container in the
`MonoImageSet:aggergate_modifiers_cache`

---

Also had to bump the AOT image format, which now always reads back in using the aggregate representation.

---

This should address #10564, #10565, #12422, #10945 (again), #6936, and the parts of #11350 that **don't** deal with arrays.

---

TODO:

- [x] Add some representative test cases to this PR.
- [ ] Modify our acceptance tests to run the various disabled Roslyn tests.
xoofx pushed a commit to stark-lang/stark-roslyn that referenced this issue Apr 16, 2019
The serialization bug has popped up again. Disabling to get CI green
while Mono takes a look.

mono/mono#10945
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Apr 24, 2019
Bumps to mono/api-snapshot@ae01378
Bumps to mono/reference-assemblies@e5173a5
Bumps to mono/bockbuild@d30329d
Bumps to mono/boringssl@3d87996
Bumps to mono/corefx@72f7d76
Bumps to mono/corert@1b7d4a1
Bumps to mono/helix-binaries@7e893ea
Bumps to mono/illinker-test-assets@f21ff68
Bumps to mono/linker@13d864e
Bumps to mono/llvm@1aaaaa5 [mono]
Bumps to mono/llvm@2c2cffe [xamarin-android]
Bumps to mono/NUnitLite@0029561
Bumps to mono/roslyn-binaries@0bbc9b4
Bumps to mono/xunit-binaries@8f6e62e

	$ git diff --shortstat 886c4901..e66c7667      # mono
        3597 files changed, 350850 insertions(+), 91128 deletions(-)
	$ git diff --shortstat 349752c464c5fc93b32e7d45825f2890c85c8b7d..2c2cffedf01e0fe266b9aaad2c2563e05b750ff4
	 240 files changed, 18562 insertions(+), 6581 deletions(-)

Context: dotnet/coreclr#22046

Fixes: CVE 2018-8292 on macOS
Fixes: http://work.devdiv.io/737323
Fixes: dotnet/corefx#33965
Fixes: dotnet/standard#642
Fixes: mono/mono#6997
Fixes: mono/mono#7326
Fixes: mono/mono#7517
Fixes: mono/mono#7750
Fixes: mono/mono#7859
Fixes: mono/mono#8360
Fixes: mono/mono#8460
Fixes: mono/mono#8766
Fixes: mono/mono#8922
Fixes: mono/mono#9418
Fixes: mono/mono#9507
Fixes: mono/mono#9951
Fixes: mono/mono#10024
Fixes: mono/mono#10030
Fixes: mono/mono#10038
Fixes: mono/mono#10448
Fixes: mono/mono#10735
Fixes: mono/mono#10735
Fixes: mono/mono#10737
Fixes: mono/mono#10743
Fixes: mono/mono#10834
Fixes: mono/mono#10837
Fixes: mono/mono#10838
Fixes: mono/mono#10863
Fixes: mono/mono#10945
Fixes: mono/mono#11020
Fixes: mono/mono#11021
Fixes: mono/mono#11021
Fixes: mono/mono#11049
Fixes: mono/mono#11091
Fixes: mono/mono#11095
Fixes: mono/mono#11123
Fixes: mono/mono#11138
Fixes: mono/mono#11146
Fixes: mono/mono#11202
Fixes: mono/mono#11214
Fixes: mono/mono#11317
Fixes: mono/mono#11326
Fixes: mono/mono#11378
Fixes: mono/mono#11385
Fixes: mono/mono#11478
Fixes: mono/mono#11479
Fixes: mono/mono#11488
Fixes: mono/mono#11489
Fixes: mono/mono#11527
Fixes: mono/mono#11529
Fixes: mono/mono#11596
Fixes: mono/mono#11603
Fixes: mono/mono#11613
Fixes: mono/mono#11623
Fixes: mono/mono#11663
Fixes: mono/mono#11681
Fixes: mono/mono#11684
Fixes: mono/mono#11693
Fixes: mono/mono#11697
Fixes: mono/mono#11779
Fixes: mono/mono#11809
Fixes: mono/mono#11858
Fixes: mono/mono#11895
Fixes: mono/mono#11898
Fixes: mono/mono#11898
Fixes: mono/mono#11965
Fixes: mono/mono#12182
Fixes: mono/mono#12193
Fixes: mono/mono#12218
Fixes: mono/mono#12235
Fixes: mono/mono#12263
Fixes: mono/mono#12307
Fixes: mono/mono#12331
Fixes: mono/mono#12362
Fixes: mono/mono#12374
Fixes: mono/mono#12402
Fixes: mono/mono#12421
Fixes: mono/mono#12461
Fixes: mono/mono#12479
Fixes: mono/mono#12479
Fixes: mono/mono#12552
Fixes: mono/mono#12603
Fixes: mono/mono#12747
Fixes: mono/mono#12831
Fixes: mono/mono#12843
Fixes: mono/mono#12881
Fixes: mono/mono#13030
Fixes: mono/mono#13284
Fixes: mono/mono#13297
Fixes: mono/mono#13455
Fixes: mono/mono#13460
Fixes: mono/mono#13478
Fixes: mono/mono#13479
Fixes: mono/mono#13522
Fixes: mono/mono#13607
Fixes: mono/mono#13610
Fixes: mono/mono#13610
Fixes: mono/mono#13639
Fixes: mono/mono#13672
Fixes: mono/mono#13834
Fixes: mono/mono#13878
Fixes: mono/mono#6352
Fixes: mono/monodevelop#6898
Fixes: xamarin/maccore#1069
Fixes: xamarin/maccore#1407
Fixes: xamarin/maccore#604
Fixes: xamarin/xamarin-macios#4984
Fixes: xamarin/xamarin-macios#5289
Fixes: xamarin/xamarin-macios#5363
Fixes: xamarin/xamarin-macios#5381
Fixes: https://issuetracker.unity3d.com/issues/editor-crashes-with-g-logv-when-entering-play-mode-with-active-flowcanvas-script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.