From 962df32fb915e80846664d994358a572df3caacc Mon Sep 17 00:00:00 2001 From: soloturn Date: Sun, 9 Aug 2026 19:58:06 +0200 Subject: [PATCH 1/3] fix: guard against null in SimpleUri(String) and StringRepresentationTypeHandler Loading a saved player entity can crash the whole game with: java.lang.NullPointerException: Cannot invoke "String.split(String, int)" because "simpleUri" is null at org.terasology.engine.core.SimpleUri.(SimpleUri.java:66) at org.terasology.engine.persistence.typeHandling.reflection.ModuleEnvironmentSandbox.doesSubclassMatch(...) ... at org.terasology.engine.persistence.typeHandling.extensionTypes.ComponentClassTypeHandler.getFromString(...) at org.terasology.persistence.typeHandling.StringRepresentationTypeHandler.deserialize(...) at org.terasology.persistence.typeHandling.coreTypes.CollectionTypeHandler.deserialize(...) ... at org.terasology.engine.persistence.internal.PlayerStoreInternal.restoreEntities(...) This happens when deserializing a persisted list of component-class references and one entry no longer resolves to a real class (e.g. a component that existed in an older engine/module build was renamed or removed) - PersistedData.isString() can report true while getAsString() still yields null for that entry, and that null was passed straight through getFromString() into "new SimpleUri(null)", which called .split() on it unconditionally. Two layered fixes: - SimpleUri(String): treat a null input the same as a malformed one - mark the URI invalid instead of throwing. SimpleUri already has this "invalid but not exceptional" behavior for non-null malformed input, this just extends it to null, consistent with the class's own documented contract ("If the string does not match this format, it will be marked invalid"). - StringRepresentationTypeHandler.deserialize(): also guard at the actual entry point where the null appears, so any subclass (not just ComponentClassTypeHandler) gets Optional.empty() instead of a null forwarded into its own getFromString(). Verified against a real save that reproduced this exact crash while testing Windows/ARM64 support (see TerasologyLauncher#727, Terasology#5359): patched the compiled classes into a running engine-5.4.0-SNAPSHOT.jar / TypeHandlerLibrary-5.4.0-SNAPSHOT.jar locally; the malformed reference is now skipped instead of crashing character spawn. Co-Authored-By: Claude Sonnet 5 --- .../java/org/terasology/engine/core/SimpleUri.java | 6 +++++- .../typeHandling/StringRepresentationTypeHandler.java | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/engine/src/main/java/org/terasology/engine/core/SimpleUri.java b/engine/src/main/java/org/terasology/engine/core/SimpleUri.java index 94c3bb59211..de6690d418a 100644 --- a/engine/src/main/java/org/terasology/engine/core/SimpleUri.java +++ b/engine/src/main/java/org/terasology/engine/core/SimpleUri.java @@ -58,11 +58,15 @@ public SimpleUri(Name moduleName, Name objectName) { } /** - * Creates a SimpleUri from a string in the format "module:object". If the string does not match this format, it will be marked invalid + * Creates a SimpleUri from a string in the format "module:object". If the string does not match this format + * (including {@code null}), it will be marked invalid. * * @param simpleUri */ public SimpleUri(String simpleUri) { + if (simpleUri == null) { + return; + } String[] split = simpleUri.split(MODULE_SEPARATOR, 2); if (split.length > 1) { moduleName = new Name(split[0]); diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java index 5d4ea3f0596..aea1613413b 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java @@ -19,7 +19,15 @@ public PersistedData serializeNonNull(T value, PersistedDataSerializer serialize @Override public Optional deserialize(PersistedData data) { if (data.isString()) { - return Optional.ofNullable(getFromString(data.getAsString())); + String value = data.getAsString(); + if (value == null) { + // A PersistedData can report isString() true while still holding no actual + // content (e.g. a stale/renamed reference in older save data) - treat that the + // same as "absent" instead of forwarding null into getFromString(), which for + // handlers like ComponentClassTypeHandler ends up constructing a SimpleUri(null). + return Optional.empty(); + } + return Optional.ofNullable(getFromString(value)); } return Optional.empty(); } From cfbc30ed787d74b8c51634fdff51b3b7da961567 Mon Sep 17 00:00:00 2001 From: Cervator Date: Sun, 9 Aug 2026 21:29:46 -0400 Subject: [PATCH 2/3] test: cover null string content in deserialize and SimpleUri MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PersistedString` returns true from `isString()` unconditionally and hands back whatever it was constructed with, so `new PersistedString(null)` reproduces the reported NPE condition with a real type rather than a mock — which also confirms the guard is reachable and not dead code. The handler test asserts `getFromString` is never entered, not just that the result is empty; a subclass tolerating null would pass the weaker assertion while the contract was broken. Co-Authored-By: Claude Opus 5 --- .../terasology/engine/core/SimpleUriTest.java | 38 +++++++++++ .../StringRepresentationTypeHandler.java | 6 +- .../StringRepresentationTypeHandlerTest.java | 66 +++++++++++++++++++ 3 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java create mode 100644 subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java diff --git a/engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java b/engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java new file mode 100644 index 00000000000..29384e72820 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/core/SimpleUriTest.java @@ -0,0 +1,38 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.core; + +import org.junit.jupiter.api.Test; +import org.terasology.gestalt.naming.Name; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SimpleUriTest { + + @Test + void parsesModuleAndObject() { + SimpleUri uri = new SimpleUri("engine:something"); + + assertTrue(uri.isValid()); + assertEquals(new Name("engine"), uri.getModuleName()); + assertEquals(new Name("something"), uri.getObjectName()); + } + + @Test + void stringWithoutSeparatorIsInvalid() { + assertFalse(new SimpleUri("engine").isValid()); + } + + @Test + void nullStringIsInvalid() { + // Regression: null reached this constructor when deserializing a stale reference and + // threw on split(). It must land in the same empty/invalid state as the no-arg form. + SimpleUri uri = new SimpleUri((String) null); + + assertFalse(uri.isValid()); + assertTrue(uri.getModuleName().isEmpty()); + assertTrue(uri.getObjectName().isEmpty()); + } +} diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java index aea1613413b..a5984d9cbca 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java @@ -21,10 +21,8 @@ public Optional deserialize(PersistedData data) { if (data.isString()) { String value = data.getAsString(); if (value == null) { - // A PersistedData can report isString() true while still holding no actual - // content (e.g. a stale/renamed reference in older save data) - treat that the - // same as "absent" instead of forwarding null into getFromString(), which for - // handlers like ComponentClassTypeHandler ends up constructing a SimpleUri(null). + // PersistedString reports isString() unconditionally, so stale save data can + // arrive with null content - treat as absent rather than call getFromString(null). return Optional.empty(); } return Optional.ofNullable(getFromString(value)); diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java new file mode 100644 index 00000000000..d0400f31dc5 --- /dev/null +++ b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandlerTest.java @@ -0,0 +1,66 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.persistence.typeHandling; + +import org.junit.jupiter.api.Test; +import org.terasology.persistence.typeHandling.inMemory.PersistedInteger; +import org.terasology.persistence.typeHandling.inMemory.PersistedString; + +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class StringRepresentationTypeHandlerTest { + + /** + * Records whether {@link #getFromString} was reached. The contract under test is not just + * "returns empty" but "never forwards null to the subclass" — asserting only on the returned + * Optional would still pass if a subclass happened to tolerate null and return null itself. + */ + private static final class RecordingHandler extends StringRepresentationTypeHandler { + private boolean getFromStringCalled; + + @Override + public String getAsString(String item) { + return item; + } + + @Override + public String getFromString(String representation) { + getFromStringCalled = true; + return representation; + } + } + + private final RecordingHandler typeHandler = new RecordingHandler(); + + @Test + void deserializesStringContent() { + Optional result = typeHandler.deserialize(new PersistedString("engine:something")); + + assertTrue(result.isPresent()); + assertEquals("engine:something", result.get()); + assertTrue(typeHandler.getFromStringCalled); + } + + @Test + void treatsNullStringContentAsAbsent() { + // PersistedString returns true from isString() unconditionally, so a null payload reaches + // deserialize() with isString() == true. That is the shape that produced the NPE: null was + // forwarded to getFromString(), and ComponentClassTypeHandler built a SimpleUri from it. + Optional result = typeHandler.deserialize(new PersistedString(null)); + + assertFalse(result.isPresent()); + assertFalse(typeHandler.getFromStringCalled, "null content must not reach getFromString()"); + } + + @Test + void ignoresNonStringData() { + Optional result = typeHandler.deserialize(new PersistedInteger(42)); + + assertFalse(result.isPresent()); + assertFalse(typeHandler.getFromStringCalled); + } +} From cf44ccf4d92905e6dcc9570fa327288d4944181d Mon Sep 17 00:00:00 2001 From: Cervator Date: Sun, 9 Aug 2026 22:02:30 -0400 Subject: [PATCH 3/3] feat: report null persisted string content once per handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Returning empty on null content trades an NPE for a component that quietly fails to restore, which is harder to diagnose than the crash was. Bounded by construction rather than by a cap: the bucket is the handler, and there are 14 subclasses, so the worst possible load emits 14 lines however many entities carry the damaged reference. No sample counters or reset window needed, unlike `WorldProviderCoreImpl.logDroppedWrite` whose bucket space is chunk positions. Per-instance, not static — MTE builds a fresh handler library per environment, and a static flag would silence every test after the first. Co-Authored-By: Claude Opus 5 --- .../StringRepresentationTypeHandler.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java index a5984d9cbca..0d84c7bc4eb 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/StringRepresentationTypeHandler.java @@ -2,10 +2,23 @@ // SPDX-License-Identifier: Apache-2.0 package org.terasology.persistence.typeHandling; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; public abstract class StringRepresentationTypeHandler extends TypeHandler { + private static final Logger logger = LoggerFactory.getLogger(StringRepresentationTypeHandler.class); + + /** + * Per-instance rather than static: a single damaged reference can appear on thousands of + * entities, so this reports once and then goes quiet - but MTE builds a fresh handler library + * per environment, and a static flag would silence every test after the first. + */ + private final AtomicBoolean warnedNullContent = new AtomicBoolean(); + public abstract String getAsString(T item); public abstract T getFromString(String representation); @@ -23,6 +36,13 @@ public Optional deserialize(PersistedData data) { if (value == null) { // PersistedString reports isString() unconditionally, so stale save data can // arrive with null content - treat as absent rather than call getFromString(null). + if (warnedNullContent.compareAndSet(false, true)) { + logger.warn("{}: persisted data reported isString() but held null content -" + + " treating it as absent. Further occurrences from this handler" + + " are silent. Usually a stale or renamed reference in older" + + " save data.", + getClass().getSimpleName()); + } return Optional.empty(); } return Optional.ofNullable(getFromString(value));