Skip to content

fix: fastbufferreader string deserialization int overflow#4052

Open
NoelStephensUnity wants to merge 16 commits into
develop-2.0.0from
fix/fastbufferreader-string-deserialization-int-overflow
Open

fix: fastbufferreader string deserialization int overflow#4052
NoelStephensUnity wants to merge 16 commits into
develop-2.0.0from
fix/fastbufferreader-string-deserialization-int-overflow

Conversation

@NoelStephensUnity

@NoelStephensUnity NoelStephensUnity commented Jun 29, 2026

Copy link
Copy Markdown
Member

Purpose of this PR

This PR resolves an edge case scenario that can cause the Unity editor to crash.
The issue requires a portion of the character count of a serialized string to have been read prior to attempting to safely or unsafely read the string. When using 2 byte character values, this can result in the length exceeding the maximum integer size resulting in a negative size being read which, in turn, causes the FastBufferReader to attempt to read into restricted memory (most likely outside of the application domain) that results in a hard editor crash.

Jira ticket

UUM-145752

Changelog

  • Fixed: Issue when FastBufferReader is attempting to read a string and all or a portion of the character count has already been read by user script, it could read a character length that results in a negative byte length which could result in an editor crash.

Documentation

  • No documentation changes or additions were necessary.

Testing & QA (How your changes can be verified during release Playtest)

Functional Testing

Manual testing :

  • Manual testing done
    • Validated against the users project in UUM-145752

Automated tests:

  • Covered by existing automated tests
  • Covered by new automated tests

Does the change require QA team to:

  • Review automated tests?
  • Execute manual tests?
  • Provide feedback about the PR?

If any boxes above are checked the QA team will be automatically added as a PR reviewer.

Backports

No back port is required.

This fixes an edge case scenario with string reading where if user code has caused the character count to have been already read prior to attempting to read a string value (safely or unsafely) and then reading the string can result in the signed integer size to roll over to a negative value and thus causing the reader to attempt to read into restricted memory outside of the application domain which results in the editor crashing.

The fix catches this scenario and throws an overflow exception prior to attempting to read into negative memory space relative to the application domain.
This test validates the fix.
Adding changelog entry.
Making the reader use the already calculated readSize.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review June 29, 2026 22:47
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner June 29, 2026 22:47
Updating test to unsafely read a string under the same condition.

@michalChrobot michalChrobot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the investigation on this!

@RikuTheFuffs RikuTheFuffs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple nitpick comments that might help with readability/maintenance, feel free to ignore them.
Apart from that LGTM!

Comment thread com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs Outdated
Comment thread com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs Outdated
Fixing an issue where the unsafe string read needs to create an empty string based on the character count and not the byte count.
@NoelStephensUnity NoelStephensUnity enabled auto-merge (squash) June 30, 2026 15:52
Based on Paolo's suggestion on combining the string size, in bytes, validation script to a single in-lined method shared between the safe and unsafe string read methods.
Also combined the actual reading of the string data into a single in-lined method.
Removing the auto-added (and not used) UnityEngine.UIElements using directive.
updated comments and renamed CheckIfValidStringLength to ValidateStringByteCount.
Removing the added 3 bytes used for earlier debugging purposes.
spelling/typo fix.
whitespace after sentence in comment fix.
Comment thread com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs Outdated
Comment thread com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs Outdated
NoelStephensUnity and others added 2 commits June 30, 2026 13:53
Based on Emma's suggestion, removing the `SizeOfLengthField` method and `TryBeginReadInternal` check within the ReadValueSafe (string) method and replacing that with `ReadLengthSafe`.
…Reader.cs

Co-authored-by: Emma <emma.mcmillan@unity3d.com>
Based on suggestion from Emma, removing InBitwiseContext check since this is done in both ReadLengthSafe and TryBeginReadInternal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants