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

[EnC] FieldInfo.GetValue on a newly-added ValueType field crashes CoreCLR #76702

Closed
lambdageek opened this issue Oct 6, 2022 · 6 comments · Fixed by #90446
Closed

[EnC] FieldInfo.GetValue on a newly-added ValueType field crashes CoreCLR #76702

lambdageek opened this issue Oct 6, 2022 · 6 comments · Fixed by #90446
Assignees
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Oct 6, 2022

Found this while working on #76462, but it also reproduces with net7 (and probably net6.0 and older, see below)

Repro:

  1. dotnet new console
  2. Use the following for Program.cs:
// #define UPDATE

using System.Reflection;

public class Test {

	public static void Main ()
	{	
		bool stop = false;

		Console.CancelKeyPress += new ((sender, ev) => {
		  ev.Cancel = true;
		  stop = true;
		});

		var c0 = new C();
		int i = 0;
		while (!stop) {
		  Body (ref i);
		}
		Console.WriteLine ("stopping");
		GC.KeepAlive (c0);
	}

	static void Body (ref int i)
	{
		Console.WriteLine($"alive {i}");
		i += 1;
		Thread.Sleep (1000);
#if UPDATE
	        var c = new C();
                var fi = c.GetType().GetField ("MyS");

		var s = fi.GetValue (c); // bad!

		Console.WriteLine ("still alive");
#endif
	}

	class C {

#if UPDATE
		public S MyS;
#endif

		public C() {
#if UPDATE
			MyS = new S {
				D = -1985.0,
				O = new int[2] {15, 17},
			};
#endif
		}

#if UPDATE
		public struct S {
			public double D;
			public object O;
		};
#endif
	}
}
  1. Run dotnet watch (but I believe this would also reproduce with EnC in Visual Studio)
  2. Change // #define UPDATE to #define UPDATE and save (or hit "Apply Changes" in VS)

Expected result:

The hot reload change is applied and the application keeps running.

Actual result:

dotnet watch 🚀 Started
alive 0
alive 1
alive 2
alive 3
alive 4
alive 5
alive 6
dotnet watch ⌚ File changed: ./Program.cs.
alive 7
dotnet watch 🔥 Hot reload of changes succeeded.
alive 8
dotnet watch ❌ Exited with error code 139
dotnet watch ⏳ Waiting for a file to change before restarting dotnet...
dotnet watch 🛑 Shutdown requested. Press Ctrl+C again to force exit.

That is, the child process - dotnet running the user code crashed with exit code 139.

In #76462 we actually get a good combination of error messages from a CoreCLR builds on OSX and Windows, respectively:

Mac:

Assert failure(PID 21236 [0x000052f4], Thread: 191148 [0x2eaac]): IS_ALIGNED(src, sizeof(SIZE_T))
    File: /Users/runner/work/1/s/src/coreclr/classlibnative/bcltype/arraynative.cpp Line: 736
    Image: /private/tmp/helix/working/B02E09C7/p/dotnet

Windows:

Fatal error. Internal CLR error. (0x80131506)
   at System.RuntimeFieldHandle.GetValue(System.Reflection.RtFieldInfo, System.Object, System.RuntimeType, System.RuntimeType, Boolean ByRef)
   at System.Reflection.RtFieldInfo.GetValue(System.Object)
   at System.Reflection.Metadata.ApplyUpdateTest+<>c.<TestAddInstanceField>b__10_0()
   at System.RuntimeMethodHandle.InvokeMethod(System.Object, Void**, System.Signature, Boolean)
   at System.Reflection.MethodInvoker.InterpretedInvoke(System.Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(System.Object, IntPtr*, System.Reflection.BindingFlags)
   at System.Reflection.MethodInvoker.InlinedInvoke(System.Object, IntPtr*, System.Reflection.BindingFlags)
   at System.Reflection.RuntimeMethodInfo.Invoke(System.Object, System.Reflection.BindingFlags, System.Reflection.Binder, System.Object[], System.Globalization.CultureInfo)
   at System.Reflection.MethodBase.Invoke(System.Object, System.Object[])
   at Microsoft.DotNet.RemoteExecutor.Program.Main(System.String[])

Which points to InvokeUtil::GetFieldValue

if (pField->IsStatic())
p = pField->GetCurrentStaticAddress();
else {
p = (*((BYTE**)target)) + pField->GetOffset() + sizeof(Object);
}
GCPROTECT_END();
// copy the field to the unboxed object.
// note: this will be done only for the non-remoting case
if (p) {
CopyValueClass(obj->GetData(), p, fieldType.AsMethodTable());

This looks wrong - p = (*((BYTE**)target)) + pField->GetOffset() + sizeof(Object); for fields added by EnC - they aren't at some offset, they're allocated entirely separately from the target object.

By the way, the same wrong logic is also in InvokeUtil::SetValidField so probably RtFieldInfo.SetField/RuntimeFieldHandle.SetField has issues too.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 6, 2022
@ghost
Copy link

ghost commented Oct 6, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Found this while working on #76462, but it also reproduces with net7 (and probably net6.0 and older, see below)

Repro:

  1. dotnet new console
  2. Use the following for Program.cs:
// #define UPDATE

using System.Reflection;

public class Test {

	public static void Main ()
	{	
		bool stop = false;

		Console.CancelKeyPress += new ((sender, ev) => {
		  ev.Cancel = true;
		  stop = true;
		});

		var c0 = new C();
		int i = 0;
		while (!stop) {
		  Body (ref i);
		}
		Console.WriteLine ("stopping");
		GC.KeepAlive (c0);
	}

	static void Body (ref int i)
	{
		Console.WriteLine($"alive {i}");
		i += 1;
		Thread.Sleep (1000);
#if UPDATE
	        var c = new C();
                var fi = c.GetType().GetField ("MyS");

		var s = fi.GetValue (c); // bad!

		Console.WriteLine ("still alive");
#endif
	}

	class C {

#if UPDATE
		public S MyS;
#endif

		public C() {
#if UPDATE
			MyS = new S {
				D = -1985.0,
				O = new int[2] {15, 17},
			};
#endif
		}

#if UPDATE
		public struct S {
			public double D;
			public object O;
		};
#endif
	}
}
  1. Run dotnet watch (but I believe this would also reproduce with EnC in Visual Studio)
  2. Change // #define UPDATE to #define UPDATE and save (or hit "Apply Changes" in VS)

Expected result:

The hot reload change is applied and the application keeps running.

Actual result:

dotnet watch 🚀 Started
alive 0
alive 1
alive 2
alive 3
alive 4
alive 5
alive 6
dotnet watch ⌚ File changed: ./Program.cs.
alive 7
dotnet watch 🔥 Hot reload of changes succeeded.
alive 8
dotnet watch ❌ Exited with error code 139
dotnet watch ⏳ Waiting for a file to change before restarting dotnet...
dotnet watch 🛑 Shutdown requested. Press Ctrl+C again to force exit.

That is, the child process - dotnet running the user code crashed with exit code 139.

In #76462 we actually get a good combination of error messages from a CoreCLR builds on OSX and Windows, respectively:

Mac:

Assert failure(PID 21236 [0x000052f4], Thread: 191148 [0x2eaac]): IS_ALIGNED(src, sizeof(SIZE_T))
    File: /Users/runner/work/1/s/src/coreclr/classlibnative/bcltype/arraynative.cpp Line: 736
    Image: /private/tmp/helix/working/B02E09C7/p/dotnet

Windows:

Fatal error. Internal CLR error. (0x80131506)
   at System.RuntimeFieldHandle.GetValue(System.Reflection.RtFieldInfo, System.Object, System.RuntimeType, System.RuntimeType, Boolean ByRef)
   at System.Reflection.RtFieldInfo.GetValue(System.Object)
   at System.Reflection.Metadata.ApplyUpdateTest+<>c.<TestAddInstanceField>b__10_0()
   at System.RuntimeMethodHandle.InvokeMethod(System.Object, Void**, System.Signature, Boolean)
   at System.Reflection.MethodInvoker.InterpretedInvoke(System.Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(System.Object, IntPtr*, System.Reflection.BindingFlags)
   at System.Reflection.MethodInvoker.InlinedInvoke(System.Object, IntPtr*, System.Reflection.BindingFlags)
   at System.Reflection.RuntimeMethodInfo.Invoke(System.Object, System.Reflection.BindingFlags, System.Reflection.Binder, System.Object[], System.Globalization.CultureInfo)
   at System.Reflection.MethodBase.Invoke(System.Object, System.Object[])
   at Microsoft.DotNet.RemoteExecutor.Program.Main(System.String[])

Which points to InvokeUtil::GetFieldValue

if (pField->IsStatic())
p = pField->GetCurrentStaticAddress();
else {
p = (*((BYTE**)target)) + pField->GetOffset() + sizeof(Object);
}
GCPROTECT_END();
// copy the field to the unboxed object.
// note: this will be done only for the non-remoting case
if (p) {
CopyValueClass(obj->GetData(), p, fieldType.AsMethodTable());

This looks wrong - pFieldData = (*((BYTE**)target)) + pField->GetOffset() + sizeof(Object); for fields added by EnC - they aren't at some offset, they're allocated entirely separately from the target object.

By the way, the same wrong logic is also in InvokeUtil::SetValidField so probably RtFieldInfo.SetField/RuntimeFieldHandle.SetField has issues too.

Author: lambdageek
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@ghost
Copy link

ghost commented Oct 6, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Found this while working on #76462, but it also reproduces with net7 (and probably net6.0 and older, see below)

Repro:

  1. dotnet new console
  2. Use the following for Program.cs:
// #define UPDATE

using System.Reflection;

public class Test {

	public static void Main ()
	{	
		bool stop = false;

		Console.CancelKeyPress += new ((sender, ev) => {
		  ev.Cancel = true;
		  stop = true;
		});

		var c0 = new C();
		int i = 0;
		while (!stop) {
		  Body (ref i);
		}
		Console.WriteLine ("stopping");
		GC.KeepAlive (c0);
	}

	static void Body (ref int i)
	{
		Console.WriteLine($"alive {i}");
		i += 1;
		Thread.Sleep (1000);
#if UPDATE
	        var c = new C();
                var fi = c.GetType().GetField ("MyS");

		var s = fi.GetValue (c); // bad!

		Console.WriteLine ("still alive");
#endif
	}

	class C {

#if UPDATE
		public S MyS;
#endif

		public C() {
#if UPDATE
			MyS = new S {
				D = -1985.0,
				O = new int[2] {15, 17},
			};
#endif
		}

#if UPDATE
		public struct S {
			public double D;
			public object O;
		};
#endif
	}
}
  1. Run dotnet watch (but I believe this would also reproduce with EnC in Visual Studio)
  2. Change // #define UPDATE to #define UPDATE and save (or hit "Apply Changes" in VS)

Expected result:

The hot reload change is applied and the application keeps running.

Actual result:

dotnet watch 🚀 Started
alive 0
alive 1
alive 2
alive 3
alive 4
alive 5
alive 6
dotnet watch ⌚ File changed: ./Program.cs.
alive 7
dotnet watch 🔥 Hot reload of changes succeeded.
alive 8
dotnet watch ❌ Exited with error code 139
dotnet watch ⏳ Waiting for a file to change before restarting dotnet...
dotnet watch 🛑 Shutdown requested. Press Ctrl+C again to force exit.

That is, the child process - dotnet running the user code crashed with exit code 139.

In #76462 we actually get a good combination of error messages from a CoreCLR builds on OSX and Windows, respectively:

Mac:

Assert failure(PID 21236 [0x000052f4], Thread: 191148 [0x2eaac]): IS_ALIGNED(src, sizeof(SIZE_T))
    File: /Users/runner/work/1/s/src/coreclr/classlibnative/bcltype/arraynative.cpp Line: 736
    Image: /private/tmp/helix/working/B02E09C7/p/dotnet

Windows:

Fatal error. Internal CLR error. (0x80131506)
   at System.RuntimeFieldHandle.GetValue(System.Reflection.RtFieldInfo, System.Object, System.RuntimeType, System.RuntimeType, Boolean ByRef)
   at System.Reflection.RtFieldInfo.GetValue(System.Object)
   at System.Reflection.Metadata.ApplyUpdateTest+<>c.<TestAddInstanceField>b__10_0()
   at System.RuntimeMethodHandle.InvokeMethod(System.Object, Void**, System.Signature, Boolean)
   at System.Reflection.MethodInvoker.InterpretedInvoke(System.Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(System.Object, IntPtr*, System.Reflection.BindingFlags)
   at System.Reflection.MethodInvoker.InlinedInvoke(System.Object, IntPtr*, System.Reflection.BindingFlags)
   at System.Reflection.RuntimeMethodInfo.Invoke(System.Object, System.Reflection.BindingFlags, System.Reflection.Binder, System.Object[], System.Globalization.CultureInfo)
   at System.Reflection.MethodBase.Invoke(System.Object, System.Object[])
   at Microsoft.DotNet.RemoteExecutor.Program.Main(System.String[])

Which points to InvokeUtil::GetFieldValue

if (pField->IsStatic())
p = pField->GetCurrentStaticAddress();
else {
p = (*((BYTE**)target)) + pField->GetOffset() + sizeof(Object);
}
GCPROTECT_END();
// copy the field to the unboxed object.
// note: this will be done only for the non-remoting case
if (p) {
CopyValueClass(obj->GetData(), p, fieldType.AsMethodTable());

This looks wrong - pFieldData = (*((BYTE**)target)) + pField->GetOffset() + sizeof(Object); for fields added by EnC - they aren't at some offset, they're allocated entirely separately from the target object.

By the way, the same wrong logic is also in InvokeUtil::SetValidField so probably RtFieldInfo.SetField/RuntimeFieldHandle.SetField has issues too.

Author: lambdageek
Assignees: -
Labels:

area-System.Reflection, area-Diagnostics-coreclr, untriaged

Milestone: -

@lambdageek
Copy link
Member Author

@tommcdon @mikelle-rogers Here's a fun one

@lambdageek
Copy link
Member Author

git blame says those lines are from the "Initial commit to populate CoreCLR repo" so... I bet this also crashes in net472

@tommcdon tommcdon added this to the 8.0.0 milestone Oct 6, 2022
@tommcdon tommcdon removed the untriaged New issue has not been triaged by the area owner label Oct 6, 2022
@tactical-drone
Copy link

tactical-drone commented Oct 14, 2022

Today I installed the latest nightly: 7.0.100 and got this:

Fatal error. Internal CLR error. (0x80131506)
   at Org.BouncyCastle.Math.EC.Rfc8032.Ed25519.ImplVerify(Byte[], Int32, Byte[], Int32, Byte[], Byte, Byte[], Int32, Int32)
   at zero.cocoon.models.CcDiscoveries+<ConsumeAsync>d__19.MoveNext()
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[[zero.cocoon.models.CcDiscoveries+<ConsumeAsync>d__19, zero.cocoon, Version=0.0.0.1, Culture=neutral, PublicKeyToken=null]](<ConsumeAsync>d__19 ByRef)
   at zero.cocoon.models.CcDiscoveries.ConsumeAsync()
   at zero.core.patterns.bushings.IoZero`1+<ConsumeAsync>d__47`1[[System.__Canon, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Boolean, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[zero.core.patterns.bushings.IoZero`1+<ConsumeAsync>d__47`1[[System.__Canon, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], zero.core, Version=0.0.0.1, Culture=neutral, PublicKeyToken=null]].MoveNext(System.Threading.Thread)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(System.Threading.Tasks.Task ByRef, System.Threading.Thread)
   at System.Threading.Tasks.Task.ExecuteEntry()
   at zero.core.runtime.scheduler.IoZeroScheduler+<HandleAsyncSchedulerTask>d__103.MoveNext()
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[zero.core.runtime.scheduler.IoZeroScheduler+<HandleAsyncSchedulerTask>d__103, zero.core, Version=0.0.0.1, Culture=neutral, PublicKeyToken=null]].MoveNext(System.Threading.Thread)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart()

I used to get this error sometimes, then it went away. Now it is back. It could be because the most up to date nugets available are 7.0.0-rtm.22511.4

To reproduce, clone https://github.com/tactical-drone/zero and run zero.sync (in RELEASE) and inside the console type boot 700.

@lambdageek
Copy link
Member Author

@tactical-drone This issue is about an Internal CoreCLR error in Edit and Continue and Hot Reload. It doesn't sound from your repro that you're using EnC. Would you mind opening a new issue?

@mikelle-rogers mikelle-rogers added the enhancement Product code improvement that does NOT require public API changes/additions label Jan 27, 2023
@tommcdon tommcdon removed the enhancement Product code improvement that does NOT require public API changes/additions label May 17, 2023
@mikelle-rogers mikelle-rogers self-assigned this Jul 14, 2023
@tommcdon tommcdon modified the milestones: 8.0.0, 9.0.0 Aug 9, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 12, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants