@dotnet-policy-service agree

Thank you for identifying this problem and sharing a solution. However, I'm concerned about making it even more possible to nest serializations, because that violates other security measures that this library makes. It's important that serializations not be nested, and in your example that you added in the test, that is a case where a class should not be deserializing its own data, it should be using a converter instead.

What is the symptom of what you saw before this fix? Maybe we can make that symptom far more obvious so people understand the problem and fix it properly.

Thanks.
In principle, I believe we should not use nesting, but it's common to accidentally create nested structures.
I feel like this unintentionally happens quite often, especially when implementing custom formatters.
I tend to make such mistakes carelessly...

I also thought ThreadStatic should be removed eventually.
Recently, the pattern of processing scratch-buffers with small stackalloc Span is widely seen in dotnet/runtime, so following that approach might not be bad.
Since it's bytes, allocating around 1024 should serve the scratch functionality well.
Since SequencePool handles ArrayPool, I don't think there's much need to take large sizes from the pool for scratch purposes.
However, the current BufferWriter is not designed that way (it cannot receive Span as scratch), so some refactoring would be necessary.

Symptom of nested serialization follows. In the test scenario NestedSerializationTest.SerializeAndDeserialize, the content of testData.SerializedKey is as follows:

AA // string (Length 12)
74 65 73 74 73 74 72 69 6E 67

The serializer for the SerializeOnSerializeClass class

• Writes an array header with size 1
• Retrieve testData.SerializedKey property
• Write byte array from retrieved testData.SerializedKey property

in serial. Therefore, the expected serialized content of testData is

91 // firarray (length 1)

C4 // bin 8 (testData.SerializedKey)
0B

AA // string (Length 12)
74 65 73 74 73 74 72 69 6E 67

However, while retrieving testData.SerializedKey property, invoking MessagePackSerializer.Serialize starts overwriting the scratchArray buffer. This results buffer just after testData.SerializedKey property retrieved:

AA // string (Length 12), overwritten by second MessagePackWriter during retrieving testData.SerializedKey
   // The first MessagePackWriter is pointing here, just after this (0xAA)

74 65 73 74 73 74 72 69 6E 67

When the serialization completes, final contents of buffer is:

AA // string (Length 12), overwritten by second MessagePackWriter during retrieving testData.SerializedKey

C4 // bin 8 (testData.SerializedKey), re-overwritten by the first MessagePackWriter
0B

AA // string (Length 12)
74 65 73 74 73 74 72 69 6E 67

This leads data malformation, unable to deserialize data.

If nested serialization is a security concern, I believe MessagePack should raise a proper exception. Using thread-local variable to track whether current thread is already serializing/deserializing should be sufficient to detect and prevent unintended nested serialization/deserialization.