From e37c131fb44a2c25e8e6f05d5f27bedbf059e7e3 Mon Sep 17 00:00:00 2001 From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Mon, 21 Aug 2023 14:29:02 +1200 Subject: [PATCH] Prevent invalid prototypes from being spawned (#4279) --- .../GameObjects/ServerEntityManager.cs | 41 ++++++++++-------- Robust.Server/GameStates/PvsSystem.cs | 42 +++++++------------ Robust.Shared/GameObjects/ComponentFactory.cs | 6 +++ Robust.Shared/GameObjects/EntityManager.cs | 13 +++++- .../GameObjects/IComponentFactory.cs | 11 +++++ .../Server/GameStates/PvsSystemTests.cs | 6 +-- .../GameState/DeletionNetworkingTests.cs | 18 ++++---- .../Physics/BroadphaseNetworkingTest.cs | 2 +- 8 files changed, 80 insertions(+), 59 deletions(-) diff --git a/Robust.Server/GameObjects/ServerEntityManager.cs b/Robust.Server/GameObjects/ServerEntityManager.cs index 82f92d2ed..a496f4927 100644 --- a/Robust.Server/GameObjects/ServerEntityManager.cs +++ b/Robust.Server/GameObjects/ServerEntityManager.cs @@ -81,30 +81,35 @@ namespace Robust.Server.GameObjects private protected override EntityUid CreateEntity(string? prototypeName, EntityUid uid = default, IEntityLoadContext? context = null) { - var entity = base.CreateEntity(prototypeName, uid, context); + if (prototypeName == null) + return base.CreateEntity(prototypeName, uid, context); - if (!string.IsNullOrWhiteSpace(prototypeName)) - { - var prototype = PrototypeManager.Index(prototypeName); + if (!PrototypeManager.TryIndex(prototypeName, out var prototype)) + throw new EntityCreationException($"Attempted to spawn an entity with an invalid prototype: {prototypeName}"); - // At this point in time, all data configure on the entity *should* be purely from the prototype. - // As such, we can reset the modified ticks to Zero, - // which indicates "not different from client's own deserialization". - // So the initial data for the component or even the creation doesn't have to be sent over the wire. - foreach (var (netId, component) in GetNetComponents(entity)) - { - // Make sure to ONLY get components that are defined in the prototype. - // Others could be instantiated directly by AddComponent (e.g. ContainerManager). - // And those aren't guaranteed to exist on the client, so don't clear them. - var compName = ComponentFactory.GetComponentName(component.GetType()); - if (prototype.Components.ContainsKey(compName)) - component.ClearTicks(); - } - } + var entity = base.CreateEntity(prototype, uid, context); + // At this point in time, all data configure on the entity *should* be purely from the prototype. + // As such, we can reset the modified ticks to Zero, + // which indicates "not different from client's own deserialization". + // So the initial data for the component or even the creation doesn't have to be sent over the wire. + ClearTicks(entity, prototype); return entity; } + private void ClearTicks(EntityUid entity, EntityPrototype prototype) + { + foreach (var (netId, component) in GetNetComponents(entity)) + { + // Make sure to ONLY get components that are defined in the prototype. + // Others could be instantiated directly by AddComponent (e.g. ContainerManager). + // And those aren't guaranteed to exist on the client, so don't clear them. + var compName = ComponentFactory.GetComponentName(netId); + if (prototype.Components.ContainsKey(compName)) + component.ClearTicks(); + } + } + public override EntityStringRepresentation ToPrettyString(EntityUid uid) { TryGetComponent(uid, out ActorComponent? actor); diff --git a/Robust.Server/GameStates/PvsSystem.cs b/Robust.Server/GameStates/PvsSystem.cs index 6d53d37fd..22b907b48 100644 --- a/Robust.Server/GameStates/PvsSystem.cs +++ b/Robust.Server/GameStates/PvsSystem.cs @@ -1034,26 +1034,20 @@ internal sealed partial class PvsSystem : EntitySystem while (query.MoveNext(out var uid, out var md)) { DebugTools.Assert(md.EntityLifeStage >= EntityLifeStage.Initialized); + DebugTools.Assert(md.EntityLifeStage < EntityLifeStage.Terminating); if (md.EntityLastModifiedTick <= fromTick) continue; var state = GetEntityState(player, uid, fromTick, md); - // Temporary debugging code. - // TODO REMOVE TEMPORARY CODE if (state.Empty) { - var msg = $"{nameof(GetEntityState)} returned an empty state while enumerating all. Entity {ToPrettyString(uid)}. Net component Data:"; - foreach (var (_, cmp) in EntityManager.GetNetComponents(uid)) - { - msg += $"\nName: {_factory.GetComponentName(cmp.GetType())}" + - $"Enabled: {cmp.NetSyncEnabled}, " + - $"Lifestage: {cmp.LifeStage}, " + - $"OwnerOnly: {cmp.SendOnlyToOwner}, " + - $"SessionSpecific: {cmp.SessionSpecific}, " + - $"LastModified: {cmp.LastModifiedTick}"; - } - Log.Error(msg); + Log.Error($@"{nameof(GetEntityState)} returned an empty state while enumerating entities. +Tick: {fromTick}--{_gameTiming.CurTick} +Entity: {ToPrettyString(uid)} +Last modified: {md.EntityLastModifiedTick} +Metadata last modified: {md.LastModifiedTick} +Transform last modified: {Transform(uid).LastModifiedTick}"); } stateEntities.Add(state); @@ -1077,26 +1071,21 @@ internal sealed partial class PvsSystem : EntitySystem continue; DebugTools.Assert(md.EntityLifeStage >= EntityLifeStage.Initialized); + DebugTools.Assert(md.EntityLifeStage < EntityLifeStage.Terminating); DebugTools.Assert(md.EntityLastModifiedTick >= md.CreationTick || md.EntityLastModifiedTick == GameTick.Zero); DebugTools.Assert(md.EntityLastModifiedTick > fromTick || md.EntityLastModifiedTick == GameTick.Zero); var state = GetEntityState(player, uid, fromTick, md); - // Temporary debugging code. - // TODO REMOVE TEMPORARY CODE if (state.Empty) { - var msg = $"{nameof(GetEntityState)} returned an empty state for new entity {ToPrettyString(uid)}. Net component Data:"; - foreach (var (_, cmp) in EntityManager.GetNetComponents(uid)) - { - msg += $"\nName: {_factory.GetComponentName(cmp.GetType())}" + - $"Enabled: {cmp.NetSyncEnabled}, " + - $"Lifestage: {cmp.LifeStage}, " + - $"OwnerOnly: {cmp.SendOnlyToOwner}, " + - $"SessionSpecific: {cmp.SessionSpecific}, " + - $"LastModified: {cmp.LastModifiedTick}"; - } - Log.Error(msg); + Log.Error($@"{nameof(GetEntityState)} returned an empty state for a new entity. +Tick: {fromTick}--{_gameTiming.CurTick} +Entity: {ToPrettyString(uid)} +Last modified: {md.EntityLastModifiedTick} +Metadata last modified: {md.LastModifiedTick} +Transform last modified: {Transform(uid).LastModifiedTick}"); + continue; } stateEntities.Add(state); @@ -1109,6 +1098,7 @@ internal sealed partial class PvsSystem : EntitySystem continue; DebugTools.Assert(md.EntityLifeStage >= EntityLifeStage.Initialized); + DebugTools.Assert(md.EntityLifeStage < EntityLifeStage.Terminating); DebugTools.Assert(md.EntityLastModifiedTick >= md.CreationTick || md.EntityLastModifiedTick == GameTick.Zero); DebugTools.Assert(md.EntityLastModifiedTick > fromTick || md.EntityLastModifiedTick == GameTick.Zero); diff --git a/Robust.Shared/GameObjects/ComponentFactory.cs b/Robust.Shared/GameObjects/ComponentFactory.cs index 111a4b7d7..7fa927371 100644 --- a/Robust.Shared/GameObjects/ComponentFactory.cs +++ b/Robust.Shared/GameObjects/ComponentFactory.cs @@ -328,6 +328,12 @@ namespace Robust.Shared.GameObjects return GetRegistration(componentType).Name; } + [Pure] + public string GetComponentName(ushort netID) + { + return GetRegistration(netID).Name; + } + public ComponentRegistration GetRegistration(ushort netID) { if (_networkedComponents is null) diff --git a/Robust.Shared/GameObjects/EntityManager.cs b/Robust.Shared/GameObjects/EntityManager.cs index 73f20abbf..0a8c66f58 100644 --- a/Robust.Shared/GameObjects/EntityManager.cs +++ b/Robust.Shared/GameObjects/EntityManager.cs @@ -751,8 +751,17 @@ namespace Robust.Shared.GameObjects if (prototypeName == null) return AllocEntity(out _, uid); - PrototypeManager.TryIndex(prototypeName, out var prototype); + if (!PrototypeManager.TryIndex(prototypeName, out var prototype)) + throw new EntityCreationException($"Attempted to spawn an entity with an invalid prototype: {prototypeName}"); + return CreateEntity(prototype, uid, context); + } + + /// + /// Allocates an entity and loads components but does not do initialization. + /// + private protected EntityUid CreateEntity(EntityPrototype prototype, EntityUid uid = default, IEntityLoadContext? context = null) + { var entity = AllocEntity(prototype, out var metadata, uid); try { @@ -764,7 +773,7 @@ namespace Robust.Shared.GameObjects // Exception during entity loading. // Need to delete the entity to avoid corrupt state causing crashes later. DeleteEntity(entity); - throw new EntityCreationException($"Exception inside CreateEntity with prototype {prototypeName}", e); + throw new EntityCreationException($"Exception inside CreateEntity with prototype {prototype.ID}", e); } } diff --git a/Robust.Shared/GameObjects/IComponentFactory.cs b/Robust.Shared/GameObjects/IComponentFactory.cs index be4fd41c1..481426f29 100644 --- a/Robust.Shared/GameObjects/IComponentFactory.cs +++ b/Robust.Shared/GameObjects/IComponentFactory.cs @@ -156,6 +156,17 @@ namespace Robust.Shared.GameObjects /// [Pure] string GetComponentName(Type componentType); + + /// + /// Gets the name of a component, throwing an exception if it does not exist. + /// + /// The network ID corresponding to the component. + /// The registered name of the component + /// + /// Thrown if no component with id exists. + /// + [Pure] + string GetComponentName(ushort netID); /// /// Gets the registration belonging to a component, throwing an exception if it does not exist. diff --git a/Robust.UnitTesting/Server/GameStates/PvsSystemTests.cs b/Robust.UnitTesting/Server/GameStates/PvsSystemTests.cs index 87314efbe..8dcc41f12 100644 --- a/Robust.UnitTesting/Server/GameStates/PvsSystemTests.cs +++ b/Robust.UnitTesting/Server/GameStates/PvsSystemTests.cs @@ -65,12 +65,12 @@ public sealed class PvsSystemTests : RobustIntegrationTest var mapCoords = new EntityCoordinates(map, new Vector2(2, 2)); await server.WaitPost(() => { - player = sEntMan.SpawnEntity("", gridCoords); - other = sEntMan.SpawnEntity("", gridCoords); + player = sEntMan.SpawnEntity(null, gridCoords); + other = sEntMan.SpawnEntity(null, gridCoords); otherXform = sEntMan.GetComponent(other); // Ensure map PVS chunk is not empty - sEntMan.SpawnEntity("", mapCoords); + sEntMan.SpawnEntity(null, mapCoords); // Attach player. var session = (IPlayerSession) sPlayerMan.Sessions.First(); diff --git a/Robust.UnitTesting/Shared/GameState/DeletionNetworkingTests.cs b/Robust.UnitTesting/Shared/GameState/DeletionNetworkingTests.cs index c961e1d19..48df3cb5f 100644 --- a/Robust.UnitTesting/Shared/GameState/DeletionNetworkingTests.cs +++ b/Robust.UnitTesting/Shared/GameState/DeletionNetworkingTests.cs @@ -84,7 +84,7 @@ public sealed class DeletionNetworkingTests : RobustIntegrationTest await server.WaitPost(() => { var coords = new EntityCoordinates(grid1, new Vector2(0.5f, 0.5f)); - player = sEntMan.SpawnEntity("", coords); + player = sEntMan.SpawnEntity(null, coords); var session = (IPlayerSession) sPlayerMan.Sessions.First(); session.AttachToEntity(player); session.JoinGame(); @@ -107,10 +107,10 @@ public sealed class DeletionNetworkingTests : RobustIntegrationTest var coords = new EntityCoordinates(grid2, new Vector2(0.5f, 0.5f)); await server.WaitPost(() => { - entA = sEntMan.SpawnEntity("", coords); - entB = sEntMan.SpawnEntity("", coords); - childA = sEntMan.SpawnEntity("", new EntityCoordinates(entA, default)); - childB = sEntMan.SpawnEntity("", new EntityCoordinates(entB, default)); + entA = sEntMan.SpawnEntity(null, coords); + entB = sEntMan.SpawnEntity(null, coords); + childA = sEntMan.SpawnEntity(null, new EntityCoordinates(entA, default)); + childB = sEntMan.SpawnEntity(null, new EntityCoordinates(entB, default)); }); await RunTicks(); @@ -122,10 +122,10 @@ public sealed class DeletionNetworkingTests : RobustIntegrationTest EntityUid clientChildB = default; await client.WaitPost(() => { - entC = cEntMan.SpawnEntity("", coords); - childC = cEntMan.SpawnEntity("", new EntityCoordinates(entC, default)); - clientChildA = cEntMan.SpawnEntity("", new EntityCoordinates(entA, default)); - clientChildB = cEntMan.SpawnEntity("", new EntityCoordinates(entB, default)); + entC = cEntMan.SpawnEntity(null, coords); + childC = cEntMan.SpawnEntity(null, new EntityCoordinates(entC, default)); + clientChildA = cEntMan.SpawnEntity(null, new EntityCoordinates(entA, default)); + clientChildB = cEntMan.SpawnEntity(null, new EntityCoordinates(entB, default)); }); await RunTicks(); diff --git a/Robust.UnitTesting/Shared/Physics/BroadphaseNetworkingTest.cs b/Robust.UnitTesting/Shared/Physics/BroadphaseNetworkingTest.cs index cb0a24d9d..c2402fed3 100644 --- a/Robust.UnitTesting/Shared/Physics/BroadphaseNetworkingTest.cs +++ b/Robust.UnitTesting/Shared/Physics/BroadphaseNetworkingTest.cs @@ -74,7 +74,7 @@ public sealed class BroadphaseNetworkingTest : RobustIntegrationTest await server.WaitPost(() => { var coords = new EntityCoordinates(grid1, new Vector2(0.5f, 0.5f)); - player = sEntMan.SpawnEntity("", coords); + player = sEntMan.SpawnEntity(null, coords); // Enable physics var physics = sEntMan.AddComponent(player);