[release/6.0] Handle parameterless ctors in structs in STJ's ReflectionEmitMemberAccessor

[EgorBo]

Backport of #62989

Customer Impact

Structs with field initializations (C#10 feature) can cause NRE or AVE exceptions(crashes), minimal repro:

JsonSerializer.Deserialize<Foo>("{}");

public struct Foo
{
    public Foo() {}
    public float A = 1;
}

causes AccessViolationException. There are two bug reports by users against 6.0: #62983 and #67781
This PR fixes this behaviour.

Testing

This PR adds tests

Risk

low

cc @eiriktsarpalis @danmoseley

[ghost]

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

Issue Details

---

Backport of #62989

Customer Impact

Structs with field initializations (C#10 feature) can cause NRE or AVE exceptions(crashes), minimal repro:

JsonSerializer.Deserialize<Foo>("{}");

public struct Foo
{
    public float A = 1;
}

causes AccessViolationException. There are two bug reports by users against 6.0: #62983 and #67781
This PR fixes this behaviour.

Testing

This PR adds tests

Risk

low

cc @eiriktsarpalis @danmoseley

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-System.Text.Json
Milestone:	-

[eiriktsarpalis]

Thanks Egor!

[JulieLeeMSFT]

@EgorBo please check the test failure. Once it is good, we will request to @jeffschwMSFT for servicing consider.

[jaredpar]

@RikkiGibson, @jcouv as Chuck is OOF

[RikkiGibson]

I was under the impression we went back and made this an error in the same release we introduced field initializers on structs. It's now required to explicitly declare at least one constructor when field initializers are present.

[EgorBo]

Good point, totally forgot about it, but if I add an explicit parameterless constructor the test case will still be valid for the issue

[jeffschwMSFT]

lgtm. @danmoseley would you also like to review?

[danmoseley]

Seems good to me. Where is the AV, though? Is that worth a separate fix, or does the JIT generally not promise to not AV on bad IL?

[jeffschwMSFT]

Unverifiable IL can easily cause the runtime and JIT to lead to crashes. No guarantees with bad IL.

[danmoseley]

is CI broken?

[carlossanlop]

@danmoseley this was missing the servicing-consider label, I added it. Needs Tactics approval.

[ericstj]

@eerhardt I see we took a similar fix in DependencyInjection in #59625. Should we backport that to 6.0 as well?

[ericstj]

For that matter, if I look at all uses of OpCodes.Newobj I see other places that don't seem to handle value types with constructors.
https://source.dot.net/#System.Private.CoreLib/OpCodes.cs,69b6a0a0bb8ecaa2,references

At least this one seems broken too:

runtime/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlDataContract.cs

Lines 277 to 312 in 5385962

	if (type.IsValueType)
	{
	System.Reflection.Emit.LocalBuilder local = ilg.DeclareLocal(type, type.Name + "Value");
	ilg.Ldloca(local);
	ilg.InitObj(type);
	ilg.Ldloc(local);
	}
	else
	{
	// Special case XElement
	// codegen the same as 'internal XElement : this("default") { }'
	ConstructorInfo ctor = GetConstructor()!;
	if (!ctor.IsPublic && type.FullName == "System.Xml.Linq.XElement")
	{
	Type? xName = type.Assembly.GetType("System.Xml.Linq.XName");
	if (xName != null)
	{
	MethodInfo? XName_op_Implicit = xName.GetMethod(
	"op_Implicit",
	BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public,
	new Type[] { typeof(string) }
	);
	ConstructorInfo? XElement_ctor = type.GetConstructor(
	BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public,
	new Type[] { xName }
	);
	if (XName_op_Implicit != null && XElement_ctor != null)
	{
	ilg.Ldstr("default");
	ilg.Call(XName_op_Implicit);
	ctor = XElement_ctor;
	}
	}
	}
	ilg.New(ctor);
	}

[eerhardt]

I see we took a similar fix in DependencyInjection in #59625. Should we backport that to 6.0 as well?

I'm not sure it would meet the bar. It was never supported in DI before, so it isn't a regression. Also I haven't seen real user feedback needing it. You also don't typically gain anything by using ValueTypes in DI since you are always boxing/unboxing. So you might as well just use a class.

So unless there is a need, I don't think it is necessary to backport to 6.0.

[carlossanlop]

@EgorBo @eiriktsarpalis @layomia @dotnet/area-system-text-json this PR is missing the OOB version bumping changes. Example: https://github.com/dotnet/runtime/pull/67104/files

In the S.T.Json.csproj, need to add this line:

<GeneratePackageOnBuild>true</GeneratePackageOnBuild>

And need to bump the ServicingVersion number to the next one:

    <ServicingVersion>INCREASE ME</ServicingVersion>

[eiriktsarpalis]

@carlossanlop done.

[carlossanlop]

Failures are unrelated: System.IO.Compression.Tests.ZipFile_Unix.UnixCreateSetsPermissionsInExternalAttributes