[NRBF] Don't use Unsafe.As when decoding DateTime(s)

[adamsitnik]

fixes #102826

[stephentoub]

Are MemberValues[0] and MemberValues[1] the same bits just typed differently?

[adamsitnik]

No. "ticks" is "dataData" with ticks mask applied:

runtime/src/libraries/System.Private.CoreLib/src/System/DateTime.cs

Line 1548 in 0f05719

public long Ticks => (long)(_dateData & TicksMask);

runtime/src/libraries/System.Private.CoreLib/src/System/DateTime.cs

Lines 1299 to 1301 in 0f05719

	// Serialize both the old and the new format
	info.AddValue(TicksField, Ticks);
	info.AddValue(DateDataField, _dateData);

[stephentoub]

Using a private constructor like this from a separate package is considered safe / supported? We have a bunch of types now that implement ISerializable but that either throw from their deserialization ctor or don't have one at all... DateTime will never be on the same plan?

[jkotas]

I agree. System.Formats.Nrbf should not depend on any of the built-in legacy infrastructure for binary serialization.

[adamsitnik]

This ctor is part of the ISerializable protocol and it's supported by other serializers as well (example: DataContractSerializer). AFAIK we have no plans to remove these ctors.

tagging @GrabYourPitchforks who suggested this solution in #102826 (comment)

[stephentoub]

AFAIK we have no plans to remove these ctors.

We've already made some of them throw PlatformNotSupportedException, e.g.

runtime/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.cs

Lines 175 to 176 in 0f05719

	protected Regex(SerializationInfo info, StreamingContext context) =>
	throw new PlatformNotSupportedException();

and entirely removed them from others, e.g.

runtime/src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs

Line 12 in 0f05719

public sealed class OperatingSystem : ISerializable, ICloneable

[jkotas]

Should we have a public API that allows us to create the specific data time value?

[adamsitnik]

Should we have a public API that allows us to create the specific data time value?

Yes, but it would not solve the problem as this package needs to support older monikers, including netstandard2.0

[jkotas]

I think both Unsafe.As and reflection are ok for existing targets. The existing targets are set in stone and we can make assumptions about them.

Unsafe.As or reflection are less than ideal for future. They limit the changes we can do in future.

[GrabYourPitchforks]

Hi all! There seems to be some confusion here, so I'll try to clarify.

For libraries which could execute on the .NET Framework runtime, we are not allowed to call APIs or access members which are undocumented. Using Unsafe.As<...>(ref datetime) as a struct overlay violates this policy. We were previously alerted to a similar problem in the SqlClient package and resolved it. See dotnet/SqlClient#1647 (comment) for additional context. Much of what I'm going to say here is basically a copy of what I posted there.

First, this requirement only applies to libraries compiled against netfx directly or against netstandard (if they can run on netfx). If you're compiled against any other TFM, go wild. :)

Second, the serialization ctor is in fact "publicly" documented by virtue of the fact that the type implements the ISerializable interface. The contract of that interface is that the type is required to implement the necessary serialization ctor - often in a nonpublic manner - and callers can absolutely rely on it being present. When running on the netfx runtime, the code sample I provided is fully supported, fully reliable, and meets all compliance requirements we're bound by.

Third, if you do continue to use a struct overlay for platforms other than netfx, ensure that on these platforms it is legal and safe for DateTime to be backed by any arbitrary bit pattern. We have been bitten in the past where overlaying atop types like decimal has led to security incidents because arbitrary untrusted bit patterns can violate an instance's internal invariants.

[jkotas]

Is it safe to depend on the order of the fields in the payload? In other words, is the exact order of fields part of the BF contract for given type?

[GrabYourPitchforks]

SerializationInfo doesn't officially document the order, but in practice it enumerates elements in the same order in which they're added, and some types are sensitive to this ordering. It's akin to how if a dictionary / hashtable changes the order of enumeration or if a sort routine changes the relative order of "equal" elements, things break.

[jkotas]

I meant it in the connection with your other comment: Is the order considered a documented .NET Framework detail that it is ok to depend on; or is the order undocumented .NET Framework detail and we should not depend on it?

My hunch is that it should be the later. The de-serializing constructor is explicitly coded to accept any order, or to accept one of the fields missing completely.

[adamsitnik]

It was my mistake to rely on the order of fields, I am going to push a fix in a minute.

[jkotas]

How does specifying tick makes us safe?

I think it can only hide problems and produce invalid values instead of throwing an exception. I cannot think about a case where it actually helps.

[adamsitnik]

I can not speak on the behalf of @GrabYourPitchforks (who authored the code), but my understanding is that initially the SerializationInfo for DateTime contained only ticks field. Later dateData was introduced, but the runtime kept emitting the old field in case it could be deserialized with older runtime.

I've double checked the code:

https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/mscorlib/system/datetime.cs#L388-L389

runtime/src/libraries/System.Private.CoreLib/src/System/DateTime.cs

Lines 837 to 839 in eb655cf

	case TicksField:
	_dateData = (ulong)Convert.ToInt64(enumerator.Value, CultureInfo.InvariantCulture);
	foundTicks = true;

And you are right, in case this code were executed on a very old runtime, we would provide an invalid result. I've removed it and added comment.

[jkotas]

• This assumes that the BF format is set is stone and that new key/value pairs won't be added in future. Is it safe assumption to make?

• If somebody constructs malicious payload with extra TimeSpan, DateTime or Guid fields or with fields of unexpected type, this pattern match won't kick in, there won't be any exception thrown and we return the raw data. Is it the desired behavior for Nrbf reader? (As far as I can tell, the reader tends to throw on anything unexpected or invalid instead of accepting it silently.)

[adamsitnik]

These are great questions.

BinaryFormatter can represent same primitive value using different record types based on the context.
In this case, when DateTime is the root object it is expressed as SystemClassWithMembersAndTypesRecord which is just a type name + key/value dictionary. In other cases, it can be represented as MemberPrimitiveTypedRecord<T> (or a raw 8 bytes).

My goal was to hide this from the end users and always map it to PrimitiveTypeRecord<T> so users don't need to become experts in this area.

This assumes that the BF format is set is stone and that new key/value pairs won't be added in future.

If we ever extend the binary representation of given types, we may need to handle the versioning here.

If somebody constructs malicious payload with extra TimeSpan, DateTime or Guid fields or with fields of unexpected type, this pattern match won't kick in, there won't be any exception thrown and we return the raw data. Is it the desired behavior for Nrbf reader?

It's allowed to create a type that is called System.DateTime and has different layout, in such cases we are going to return a ClassRecord and the users will need to handle it.

SerializationRecord rootObject = NrbfDecoder.Decode(payload);
if (rootObject is PrimitiveTypeRecord<DateTime> primitiveRecord)
{
    // DateTime
}
else if (rootObject is ClassRecord classRecord)
{
    // something else
}

@jkotas this is just the way I see it, please let me know if something is not clear or some other changes are needed

[jkotas]

My goal was to hide this from the end users and always map it to PrimitiveTypeRecord

You are not always mapping it to PrimitiveTypeRecord<T>.

You are only mapping it to PrimitiveTypeRecord<T> if the input has specific shape. You are not mapping it for all possible valid input shapes. For example, if the payload was produced by .NET Framework 1.x (I am sure there are a bunch of such payloads still alive in the wild), it will be missing dateData field and it is not going to be mapped. However, the classic BF deserializer is going to handle it just fine. If somebody runs into this case, they will have to do double the work: They will need to handle both mapped and the non-mapped cases.

In general, I would expect the behavior to be either:

• 100% compatible with classic BF deserializer
• Exception to be thrown