-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix ReadOnlyMemory/Memory deserialization during stream continuation with null elements #121411
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
base: main
Are you sure you want to change the base?
Conversation
Add IsContinuation check to prevent early return when resuming from continuation state with null tokens Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Memory.cs
Show resolved
Hide resolved
…umption Changed from !state.IsContinuation to state.Current.ObjectState == StackFrameObjectState.None for proper resumption handling. Added tests with 200 nulls and DefaultBufferSize=1 as requested. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Memory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/Common/CollectionTests/CollectionTests.Memory.cs
Outdated
Show resolved
Hide resolved
Simplified null array generation using Enumerable.Repeat instead of Array.Fill as suggested in code review. Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the deserialization of Memory<T> and ReadOnlyMemory<T> when deserializing from streams containing arrays with many null elements. The issue occurred because the converters would incorrectly treat null array elements as null collections during streaming deserialization with resumptions.
- Modified
MemoryConverter<T>andReadOnlyMemoryConverter<T>to checkstate.Current.ReturnValuebefore treating null tokens as null collections - Added regression tests for both
Memory<string?>andReadOnlyMemory<string?>deserialization from streams with 200 null elements - Added
JsonSerializableattributes to registerMemory<string>andReadOnlyMemory<string>types for source generation tests
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ReadOnlyMemoryConverter.cs | Fixed null handling to distinguish between null collection and null array elements during streaming deserialization |
| MemoryConverter.cs | Fixed null handling to distinguish between null collection and null array elements during streaming deserialization |
| CollectionTests.Memory.cs | Added regression tests for deserializing Memory/ReadOnlyMemory with many null elements from streams |
| CollectionTests.cs (SourceGeneration) | Registered Memory and ReadOnlyMemory types for source generation test contexts |
Fix JsonSerializer.Deserialize for ReadOnlyMemory and Memory in streaming scenarios
Problem
When deserializing
ReadOnlyMemory<T>orMemory<T>from a stream, the converters incorrectly return early when resuming from continuation state if the current token is null, treating it as a root-level null instead of a null array element.Solution
Changed the null check in
OnTryReadto usestate.Current.ObjectState == StackFrameObjectState.Noneinstead of!state.IsContinuation. This properly distinguishes between:Changes made:
if (reader.TokenType is JsonTokenType.Null && !state.IsContinuation)toif (reader.TokenType is JsonTokenType.Null && state.Current.ObjectState == StackFrameObjectState.None)if (reader.TokenType is JsonTokenType.Null && !state.IsContinuation)toif (reader.TokenType is JsonTokenType.Null && state.Current.ObjectState == StackFrameObjectState.None)DeserializeReadOnlyMemoryFromStreamWithManyNulls: Tests with 200 null elements and DefaultBufferSize=1DeserializeMemoryFromStreamWithManyNulls: Tests with 200 null elements and DefaultBufferSize=1Enumerable.Repeatfor clean test data generationTesting
Technical Details
The fix uses
ObjectState == Noneinstead of!IsContinuationbecause:ObjectState == None, we're at the very beginning before any processing has started - a null token means the root value is nullObjectState != None, we've already started processing the collection - a null token is likely a null element in the array, not a root-level nullThis properly handles resumption at arbitrary reader positions during streaming deserialization with small buffer sizes.
Security Summary
No security vulnerabilities were introduced or discovered during this change.
Fixes #118346
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.