Skip to content

fix!: NativeList<T> causes crashes when used in RPCs [NCCBUG-106] #1901

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

Merged
merged 21 commits into from
Apr 27, 2022

Conversation

ShadauxCat
Copy link
Collaborator

@ShadauxCat ShadauxCat commented Apr 25, 2022

There were two issues here:

  • Any generic type that was passed to an RPC would result in a native crash because the IL code generated was invalid
  • Even with valid IL code, passing NativeList to an RPC would result in a crash because the value being sent over the wire would just be the pointer address, which would not point to valid memory on the receiving system

To address this, two things have been done:

  • The ILPP code has been fixed so it generates correct IL code with generics
  • RPCs (and FastBufferWriter/Reader) are now limited in what they'll accept. Specifically, they'll accept:
    • Primitive types (int, bool, byte, etc)
    • unmanaged enums
    • Structs or classes that implement INetworkSerializable
    • unmanaged structs that are tagged as ISerializeByMemcpy (an empty interface that the developer can add to their struct to indicate that it's compatible with memcpy serialization and can be sent over the wire)
    • Unity built-in struct types (Vector3, Color, etc)
    • Any type for which extension methods for FastBufferWriter and FastBufferReader have been created to enable them to be serialized

For types that aren't natively supported but aren't in the developer's control to be able to add ISerializeByMemcpy to it, there is ForceSerializeByMemcpy, a wrapper that adds the tag while supporting implicit conversion to and from its wrapped type. It has the limitation that, in order to be supported by NativeList, the type it wraps must be both unmanaged and IEquatable<T>.

Currently, FixedString is not in the list of natively-supported types (due to the number of varying sizes it can have) and must be wrapped in ForceSerializeByMemcpy to be used.

Changelog

  • Fixed: Passing generic types to RPCs no longer causes a native crash
  • Changed: unmanaged structs are no longer universally accepted as RPC parameters because some structs (i.e., structs with pointers in them, such as NativeList<T>) can't be supported by the universal struct serializer. Structs that are intended to be serialized across the network must add ISerializeByMemcpy to the interface list (i.e., struct Foo : ISerializeByMemcpy). This interface is empty and just serves to mark the struct as compatible with universal serialization. For external structs you can't edit, you can pass them to RPCs by wrapping them in ForceSerializeByMemcpy<T>.

Testing and Documentation

  • Includes integration tests... but more, better tests need to be added
  • Needs edits to existing public API documentation.

…without a ForceSerializeByMemcpy wrapper due to the ISerializeByMemcpy tag requirement)

- Made sure there's at least one test exercising Unity types going through RPCs... more tests exercising all of these are needed later.
# Conflicts:
#	com.unity.netcode.gameobjects/Runtime/Messaging/Messages/SnapshotDataMessage.cs
#	com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs
#	testproject/Packages/packages-lock.json
@ShadauxCat ShadauxCat changed the title Fix/native list crashes when used in RPCs [NCCBUG-106] fix: native list crashes when used in RPCs [NCCBUG-106] Apr 25, 2022
@ShadauxCat ShadauxCat changed the title fix: native list crashes when used in RPCs [NCCBUG-106] fix: NativeList<T> crashes when used in RPCs [NCCBUG-106] Apr 25, 2022
@ShadauxCat ShadauxCat changed the title fix: NativeList<T> crashes when used in RPCs [NCCBUG-106] fix: NativeList<T> causes crashes when used in RPCs [NCCBUG-106] Apr 25, 2022
@ShadauxCat ShadauxCat requested a review from a team as a code owner April 25, 2022 17:31
@@ -10,6 +10,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
### Added

### Changed
- `unmanaged` structs are no longer universally accepted as RPC parameters because some structs (i.e., structs with pointers in them, such as `NativeList`) can't be supported by the universal struct serializer. Structs that are intended to be serialized across the network must add `ISerializeByMemcpy` to the interface list (i.e., `struct Foo : ISerializeByMemcpy`). This interface is empty and just serves to mark the struct as compatible with universal serialization. For external structs you can't edit, you can pass them to RPCs by wrapping them in `ForceSerializeByMemcpy<T>`. (#1901)
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't this also mean that any existing code relying on a struct being serialized without the interface will immediately break after this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. But there's really no way to make NativeList <T> an error without making other things an error. We can choose to leave this part of it out, in which case we can fix template types causing crashes, but people will still be able to pass in NativeList and it will still crash... the alternative to doing this is documentation saying not to use NativeList in RPCs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or we could add explicit support for NativeList and then that'll work, but NativeArray will crash... we can support all the things but there will always be something users can pass in without realizing it's invalid that will cause a crash.

@0xFA11 0xFA11 changed the title fix: NativeList<T> causes crashes when used in RPCs [NCCBUG-106] fix!: NativeList<T> causes crashes when used in RPCs [NCCBUG-106] Apr 25, 2022
…ializers for by looking for instances of NetworkVariable<T> and NetworkList<T>. This was required because runtime reflection doesn't work in AoT consoles; the runtime reflection will now only ever be needed if someone does something particularly odd like, for some reason, making a NetworkVariable or NetworkList that's not a field of a NetworkBehaviour.

Made an class to go between NetworkVariable and NetworkVariableBase so that future uses for these serialization functions will also be picked up automatically by ILPP.

Also, review feedback:
- Renamed ISerializeByMemcpy to INetworkSerializeByMemcpy
- Reverted Burst json file
- Removed an errant empty line
…_in_RPCs' into fix/NativeList_crashes_when_used_in_RPCs
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.

LGTM!
Other than the standards check issue. 😸


namespace Unity.Netcode
{
public struct ForceNetworkSerializeByMemcpy<T> : INetworkSerializeByMemcpy, IEquatable<ForceNetworkSerializeByMemcpy<T>> where T : unmanaged, IEquatable<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

we need xmldoc


namespace Unity.Netcode
{
public interface INetworkSerializeByMemcpy
Copy link
Contributor

Choose a reason for hiding this comment

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

we need xmldoc

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.

we need xmldoc for new public ForceNetworkSerializeByMemcpy & INetworkSerializeByMemcpy types

Comment on lines +780 to +784
// These structs enable overloading of WriteValue with different generic constraints.
// The compiler's actually able to distinguish between overloads based on generic constraints.
// But at the bytecode level, the constraints aren't included in the method signature.
// By adding a second parameter with a defaulted value, the signatures of each generic are different,
// thus allowing overloads of methods based on the first parameter meeting constraints.
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be an xmldoc (aka 3 slashes summary block ///)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't intend it to be since it's referring to a whole section and not a specific struct or method.

I wasn't really sure what to do with xmldocs for this section because the methods are one line each and adding a 5 line xmldoc for each one seemed overkill.

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.

gtg 🚀

@ShadauxCat ShadauxCat enabled auto-merge (squash) April 27, 2022 17:30
@ShadauxCat ShadauxCat merged commit 38a1510 into develop Apr 27, 2022
@ShadauxCat ShadauxCat deleted the fix/NativeList_crashes_when_used_in_RPCs branch April 27, 2022 18:11
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.

6 participants