Skip to content

fix: implement warning logs on non-owner invoking ServerRpc that requires ownership #627

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

Merged
merged 2 commits into from
Mar 16, 2021
Merged

fix: implement warning logs on non-owner invoking ServerRpc that requires ownership #627

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e39208b
add todo comments
0xFA11 Mar 14, 2021
7ccb4ef
fix: implement warning logs on non-owner invoking ServerRpc that requ…
0xFA11 Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 78 additions & 2 deletions com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Linq;
using System.Collections.Generic;
using System.Reflection;
using MLAPI.Logging;
using MLAPI.Messaging;
using MLAPI.Serialization;
using Mono.Cecil;
Expand Down Expand Up @@ -74,12 +75,14 @@ public override ILPostProcessResult Process(ICompiledAssembly compiledAssembly)
return new ILPostProcessResult(new InMemoryAssembly(pe.ToArray(), pdb.ToArray()), m_Diagnostics);
}

private MethodReference Debug_LogWarning_MethodRef;
private TypeReference NetworkManager_TypeRef;
private MethodReference NetworkManager_getLocalClientId_MethodRef;
private MethodReference NetworkManager_getIsListening_MethodRef;
private MethodReference NetworkManager_getIsHost_MethodRef;
private MethodReference NetworkManager_getIsServer_MethodRef;
private MethodReference NetworkManager_getIsClient_MethodRef;
private FieldReference NetworkManager_LogLevel_FieldRef;
private FieldReference NetworkManager_ntable_FieldRef;
private MethodReference NetworkManager_ntable_Add_MethodRef;
private TypeReference NetworkBehaviour_TypeRef;
Expand Down Expand Up @@ -142,11 +145,13 @@ public override ILPostProcessResult Process(ICompiledAssembly compiledAssembly)
private MethodReference NetworkSerializer_SerializeRayArray_MethodRef;
private MethodReference NetworkSerializer_SerializeRay2DArray_MethodRef;

private const string k_Debug_LogWarning = nameof(Debug.LogWarning);
private const string k_NetworkManager_LocalClientId = nameof(NetworkManager.LocalClientId);
private const string k_NetworkManager_IsListening = nameof(NetworkManager.IsListening);
private const string k_NetworkManager_IsHost = nameof(NetworkManager.IsHost);
private const string k_NetworkManager_IsServer = nameof(NetworkManager.IsServer);
private const string k_NetworkManager_IsClient = nameof(NetworkManager.IsClient);
private const string k_NetworkManager_LogLevel = nameof(NetworkManager.LogLevel);
#pragma warning disable 618
private const string k_NetworkManager_ntable = nameof(NetworkManager.__ntable);

Expand All @@ -170,6 +175,17 @@ public override ILPostProcessResult Process(ICompiledAssembly compiledAssembly)

private bool ImportReferences(ModuleDefinition moduleDefinition)
{
var debugType = typeof(Debug);
foreach (var methodInfo in debugType.GetMethods())
{
switch (methodInfo.Name)
{
case k_Debug_LogWarning:
if (methodInfo.GetParameters().Length == 1) Debug_LogWarning_MethodRef = moduleDefinition.ImportReference(methodInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use MLAPIs networklog like pretty much everything else in our codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean doesn't it just goto LogWarning anyway and just add a "[MLAPI]" prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but if it is a client logging the warning (which is the case here) then it will also send a network message to the server to display the log there as well. Since this method looks like something we might reuse later for other ILPP stuff we might want to get this right to have logging consistency? Not sure if it is really needed.

break;
}
}

var networkManagerType = typeof(NetworkManager);
NetworkManager_TypeRef = moduleDefinition.ImportReference(networkManagerType);
foreach (var propertyInfo in networkManagerType.GetProperties())
Expand Down Expand Up @@ -198,6 +214,9 @@ private bool ImportReferences(ModuleDefinition moduleDefinition)
{
switch (fieldInfo.Name)
{
case k_NetworkManager_LogLevel:
NetworkManager_LogLevel_FieldRef = moduleDefinition.ImportReference(fieldInfo);
break;
case k_NetworkManager_ntable:
NetworkManager_ntable_FieldRef = moduleDefinition.ImportReference(fieldInfo);
NetworkManager_ntable_Add_MethodRef = moduleDefinition.ImportReference(fieldInfo.FieldType.GetMethod("Add"));
Expand Down Expand Up @@ -598,7 +617,7 @@ private void InjectWriteAndCallBlocks(MethodDefinition methodDefinition, CustomA
var roReturnInstr = processor.Create(OpCodes.Ret);
var roLastInstr = processor.Create(OpCodes.Nop);

// if (this.OwnerClientId != networkManager.LocalClientId) return;
// if (this.OwnerClientId != networkManager.LocalClientId) { ... } return;
instructions.Add(processor.Create(OpCodes.Ldarg_0));
instructions.Add(processor.Create(OpCodes.Call, NetworkBehaviour_getOwnerClientId_MethodRef));
instructions.Add(processor.Create(OpCodes.Ldloc, netManLocIdx));
Expand All @@ -608,6 +627,23 @@ private void InjectWriteAndCallBlocks(MethodDefinition methodDefinition, CustomA
instructions.Add(processor.Create(OpCodes.Ceq));
instructions.Add(processor.Create(OpCodes.Brfalse, roLastInstr));

var logNextInstr = processor.Create(OpCodes.Nop);

// if (LogLevel.Normal > networkManager.LogLevel)
instructions.Add(processor.Create(OpCodes.Ldloc, netManLocIdx));
instructions.Add(processor.Create(OpCodes.Ldfld, NetworkManager_LogLevel_FieldRef));
instructions.Add(processor.Create(OpCodes.Ldc_I4, (int)LogLevel.Normal));
instructions.Add(processor.Create(OpCodes.Cgt));
instructions.Add(processor.Create(OpCodes.Ldc_I4, 0));
instructions.Add(processor.Create(OpCodes.Ceq));
instructions.Add(processor.Create(OpCodes.Brfalse, logNextInstr));

// Debug.LogWarning(...);
instructions.Add(processor.Create(OpCodes.Ldstr, "Only the owner can invoke a ServerRpc that requires ownership!"));
instructions.Add(processor.Create(OpCodes.Call, Debug_LogWarning_MethodRef));

instructions.Add(logNextInstr);

instructions.Add(roReturnInstr);
instructions.Add(roLastInstr);
}
Expand Down Expand Up @@ -1543,13 +1579,36 @@ private MethodDefinition GenerateStaticHandler(MethodDefinition methodDefinition
}

nhandler.Body.InitLocals = true;
// NetworkManager networkManager;
nhandler.Body.Variables.Add(new VariableDefinition(NetworkManager_TypeRef));
int netManLocIdx = nhandler.Body.Variables.Count - 1;

{
var returnInstr = processor.Create(OpCodes.Ret);
var lastInstr = processor.Create(OpCodes.Nop);

// networkManager = this.NetworkManager;
processor.Emit(OpCodes.Ldarg_0);
processor.Emit(OpCodes.Call, NetworkBehaviour_getNetworkManager_MethodRef);
processor.Emit(OpCodes.Stloc, netManLocIdx);

// if (networkManager == null || !networkManager.IsListening) return;
processor.Emit(OpCodes.Ldloc, netManLocIdx);
processor.Emit(OpCodes.Brfalse, returnInstr);
processor.Emit(OpCodes.Ldloc, netManLocIdx);
processor.Emit(OpCodes.Callvirt, NetworkManager_getIsListening_MethodRef);
processor.Emit(OpCodes.Brtrue, lastInstr);

processor.Append(returnInstr);
processor.Append(lastInstr);
}

if (isServerRpc && requireOwnership)
{
var roReturnInstr = processor.Create(OpCodes.Ret);
var roLastInstr = processor.Create(OpCodes.Nop);

// if (rpcParams.Server.Receive.SenderClientId != target.OwnerClientId) return;
// if (rpcParams.Server.Receive.SenderClientId != target.OwnerClientId) { ... } return;
processor.Emit(OpCodes.Ldarg_2);
processor.Emit(OpCodes.Ldfld, RpcParams_Server_FieldRef);
processor.Emit(OpCodes.Ldfld, ServerRpcParams_Receive_FieldRef);
Expand All @@ -1561,6 +1620,23 @@ private MethodDefinition GenerateStaticHandler(MethodDefinition methodDefinition
processor.Emit(OpCodes.Ceq);
processor.Emit(OpCodes.Brfalse, roLastInstr);

var logNextInstr = processor.Create(OpCodes.Nop);

// if (LogLevel.Normal > networkManager.LogLevel)
processor.Emit(OpCodes.Ldloc, netManLocIdx);
processor.Emit(OpCodes.Ldfld, NetworkManager_LogLevel_FieldRef);
processor.Emit(OpCodes.Ldc_I4, (int)LogLevel.Normal);
processor.Emit(OpCodes.Cgt);
processor.Emit(OpCodes.Ldc_I4, 0);
processor.Emit(OpCodes.Ceq);
processor.Emit(OpCodes.Brfalse, logNextInstr);

// Debug.LogWarning(...);
processor.Emit(OpCodes.Ldstr, "Only the owner can invoke a ServerRpc that requires ownership!");
processor.Emit(OpCodes.Call, Debug_LogWarning_MethodRef);

processor.Append(logNextInstr);

processor.Append(roReturnInstr);
processor.Append(roLastInstr);
}
Expand Down