Skip to content

fix: corrected NetworkVariable WriteField/WriteDelta/ReadField/ReadDelta dropping the last byte if unaligned. #1008

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 9, 2021
Merged

fix: corrected NetworkVariable WriteField/WriteDelta/ReadField/ReadDelta dropping the last byte if unaligned. #1008

merged 6 commits into from
Aug 9, 2021

Conversation

ShadauxCat
Copy link
Collaborator

@ShadauxCat ShadauxCat commented Aug 2, 2021 •
edited
Loading

Added a test to specifically exercise this.

Thanks to @NoelStephensUnity for pointing out the flaw in my original fix attempt.

@ShadauxCat
fix: corrected an off-by-one copying the netvar buffer into NetworkVa…
8759d97 
…riableDelta message

Other tests missed this because they were sending small numbers and the extra space in the receive buffer was initialized with 0s (i.e., in little endian, a 32 bit number representing 1 would be 01 00 00 00, if you lose the last byte you've lost 00, when the buffer is initialized with 0s, you happen to get the correct byte back).

Added a test to specifically exercise this.
0xFA11
0xFA11 approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

thanks for the tests! 😎

0xFA11
0xFA11 reviewed Aug 2, 2021
View reviewed changes
0xFA11
0xFA11 reviewed Aug 2, 2021
View reviewed changes
0xFA11
0xFA11 suggested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

you never run ./standards.py --check locally, do you? :)

@ShadauxCat
Copy link
Collaborator Author

you never run ./standards.py --check locally, do you? :)

I tried to once and got a bunch of errors about dotnet-format not being a recognized command. At the time it was just easier to fix the issues than to try to figure out how to set that up... then I forgot about it.

@ShadauxCat ShadauxCat closed this Aug 2, 2021
@ShadauxCat ShadauxCat reopened this Aug 2, 2021
@ShadauxCat ShadauxCat changed the title fix: corrected an off-by-one copying the netvar buffer into NetworkVariableDelta message fix: corrected NetworkVariable WriteField/WriteDelta/ReadField/ReadDelta dropping the last byte if unaligned. Aug 2, 2021
mattwalsh-unity
mattwalsh-unity reviewed Aug 3, 2021
View reviewed changes
com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs
@@ -826,6 +826,7 @@ internal static void WriteNetworkVariableData(List<INetworkVariable> networkVari
else
{
networkVariableList[j].WriteField(stream);
writer.WritePadBits();
Copy link
Contributor

@mattwalsh-unity mattwalsh-unity Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting. Did we find a bug here? Or is there an encoding change? I'm actually using this PR to educate myself on where we write padding.

Copy link
Collaborator Author

@ShadauxCat ShadauxCat Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So the issue here was not what I initially thought... it was that NetworkList was writing 3 bits at the very end of the buffer when it wrote EventType.Clear, and when there's an unaligned write left at the end of the buffer, the Position value ends up being one byte too small to get those last 3 bits. This just makes sure the buffer is aligned after the write.

There's no specific case in WriteField that's doing this right now, but my figuring is, if the bug has happened in WriteDelta, we should future-proof WriteField against it, too... otherwise WriteField is a landmine waiting to explode some day.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like on our offline convo about just banishing bitwise writing (or having a wrapper which lets you bitwise write but then it pads it for you) to prevent more weird fragile cases like this and because we've learned from other netcode folks the wisdom of not being over-the-top-miserly on space. In the meantime, if this shores us up, great

0xFA11
0xFA11 approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

scary landmines

NoelStephensUnity
NoelStephensUnity approved these changes Aug 4, 2021
View reviewed changes
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Just need to fix the merge issues, merge again, and it should be good to go!

@ShadauxCat ShadauxCat enabled auto-merge (squash) August 9, 2021 19:58
@ShadauxCat ShadauxCat merged commit 40a6aec into develop Aug 9, 2021
@ShadauxCat ShadauxCat deleted the fix/off_by_one_netvar_buffer_copy branch August 9, 2021 20:08
SamuelBellomo added a commit that referenced this pull request Aug 11, 2021
@SamuelBellomo
Merge branch 'develop' into sam/feature/interpolation-for-network-tra…
b776957 
…nsform

* develop: (32 commits)
  refactor: calling networkShow(NetworkObject) code in networkshow(List<NetworkObject>) (#1028)
  feat: snapshot. MTT-685 MTT-822 (#1021)
  test: adding a multi-instance test checking NetworkShow and NetworkHide on lists of objects (#1036)
  fix: corrected NetworkVariable WriteField/WriteDelta/ReadField/ReadDelta dropping the last byte if unaligned. (#1008)
  chore: run standards check over solution files (#1027)
  chore: replace MLAPI with Netcode in Markdown files (#1025)
  fix!: added plainly-callable Add() method to NetworkSet [MTT-1005] (#1022)
  fix: fixing incorrect merge done as part of commit 85842ee (#1023)
  chore: cleanup/upgrade serialized scenes (#1020)
  chore: replace MLAPI with Netcode in C# source files (#1019)
  test: add network collections, struct and class tests MTT-936 (#1000)
  test: add buildtests to test build pipeline on target platforms (#1018)
  chore: rename MLAPI types to Netcode (#1017)
  chore!: rename asmdefs, change top-level namespaces (#1015)
  Replacing community NetworkManagerHUD with a simpler implementation (#993)
  test: network prefab pools and INetworkPrefabInstanceHandler (#1004)
  fix: do not expose Runtime internals to TestProject.ManualTests asmdef (#1014)
  refactor: snapshot. merge preparation. Removing old acks, removing unused varia… (#1013)
  chore!: per-asmdef namespaces instead of per-folder (#1009)
  feat: snapshot. ground work, preparing depedencies. No impact on code behaviour (#1012)
  ...

# Conflicts:
#	com.unity.multiplayer.mlapi/Prototyping/NetworkTransform.cs
#	com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageHandler.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFA11 0xFA11 0xFA11 approved these changes

@mattwalsh-unity mattwalsh-unity mattwalsh-unity approved these changes

@NoelStephensUnity NoelStephensUnity NoelStephensUnity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShadauxCat @0xFA11 @mattwalsh-unity @NoelStephensUnity