From 0069591af99a5805deef94506e0646674db91f92 Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Sun, 25 Aug 2024 09:24:10 +0100 Subject: [PATCH] Fix overflow when converting recursive objects to Lua MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In cdcd82679c8e286738afd6f19d1d9ed845dcd2aa, we rewrote the Lua conversion function to update the "Java -> Lua" mapping after conversion, rather than part way through. This made the code a little cleaner (as we only updated the mapping in one place), but is entirely incorrect — we need to store the object first, in order to correctly handle recursive objects — otherwise we'll just recurse infinitely (or until we overflow). This partially reverts the above commit, while preserving the new behaviour for singleton collections. Fixes #1955. --- .../core/lua/CobaltLuaMachine.java | 84 ++++++++++--------- .../computercraft/core/util/LuaUtil.java | 17 +++- .../resources/test-rom/spec/apis/os_spec.lua | 8 ++ 3 files changed, 66 insertions(+), 43 deletions(-) diff --git a/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java b/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java index 5d51e55e0..e0f2ccf17 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java +++ b/projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java @@ -95,11 +95,8 @@ public class CobaltLuaMachine implements ILuaMachine { private void addAPI(LuaState state, LuaTable globals, ILuaAPI api) throws LuaError { // Add the methods of an API to the global table - var table = wrapLuaObject(api); - if (table == null) { - LOG.warn("API {} does not provide any methods", api); - table = new LuaTable(); - } + var table = new LuaTable(); + if (!makeLuaObject(api, table)) LOG.warn("API {} does not provide any methods", api); var names = api.getNames(); for (var name : names) globals.rawset(name, table); @@ -163,13 +160,16 @@ public class CobaltLuaMachine implements ILuaMachine { timeout.removeListener(timeoutListener); } - @Nullable - private LuaTable wrapLuaObject(Object object) { - var table = new LuaTable(); - var found = luaMethods.forEachMethod(object, (target, name, method, info) -> + /** + * Populate a table with methods from an object. + * + * @param object The object to draw methods from. + * @param table The table to fill. + * @return Whether any methods were found. + */ + private boolean makeLuaObject(Object object, LuaTable table) { + return luaMethods.forEachMethod(object, (target, name, method, info) -> table.rawset(name, new ResultInterpreterFunction(this, method, target, context, name))); - - return found ? table : null; } private LuaValue toValue(@Nullable Object object, @Nullable IdentityHashMap values) throws LuaError { @@ -184,47 +184,35 @@ public class CobaltLuaMachine implements ILuaMachine { return ValueFactory.valueOf(bytes); } - // Don't share singleton values, and instead convert them to a new table. - if (LuaUtil.isSingletonCollection(object)) return new LuaTable(); - + // We have a more complex object, which is possibly recursive. First look up our object in the lookup map, + // and reuse it if present. if (values == null) values = new IdentityHashMap<>(1); var result = values.get(object); if (result != null) return result; - var wrapped = toValueWorker(object, values); - if (wrapped == null) { - LOG.warn(Logging.JAVA_ERROR, "Received unknown type '{}', returning nil.", object.getClass().getName()); - return Constants.NIL; - } - - values.put(object, wrapped); - return wrapped; - } - - /** - * Convert a complex Java object (such as a collection or Lua object) to a Lua value. - *

- * This is a worker function for {@link #toValue(Object, IdentityHashMap)}, which handles the actual construction - * of values, without reading/writing from the value map. - * - * @param object The object to convert. - * @param values The map of Java to Lua values. - * @return The converted value, or {@code null} if it could not be converted. - * @throws LuaError If the value could not be converted. - */ - private @Nullable LuaValue toValueWorker(Object object, IdentityHashMap values) throws LuaError { if (object instanceof ILuaFunction) { - return new ResultInterpreterFunction(this, FUNCTION_METHOD, object, context, object.toString()); + var function = new ResultInterpreterFunction(this, FUNCTION_METHOD, object, context, object.toString()); + values.put(object, function); + return function; } if (object instanceof IDynamicLuaObject) { - LuaValue wrapped = wrapLuaObject(object); - if (wrapped == null) wrapped = new LuaTable(); - return wrapped; + var table = new LuaTable(); + makeLuaObject(object, table); + values.put(object, table); + return table; } + // The following objects may be recursive. In these instances, we need to be careful to store the value *before* + // recursing, to avoid stack overflows. + if (object instanceof Map map) { + // Don't share singleton values, and instead convert them to a new table. + if (LuaUtil.isSingletonMap(map)) return new LuaTable(); + var table = new LuaTable(); + values.put(object, table); + for (var pair : map.entrySet()) { var key = toValue(pair.getKey(), values); var value = toValue(pair.getValue(), values); @@ -234,7 +222,12 @@ public class CobaltLuaMachine implements ILuaMachine { } if (object instanceof Collection objects) { + // Don't share singleton values, and instead convert them to a new table. + if (LuaUtil.isSingletonCollection(objects)) return new LuaTable(); + var table = new LuaTable(objects.size(), 0); + values.put(object, table); + var i = 0; for (var child : objects) table.rawset(++i, toValue(child, values)); return table; @@ -242,11 +235,20 @@ public class CobaltLuaMachine implements ILuaMachine { if (object instanceof Object[] objects) { var table = new LuaTable(objects.length, 0); + values.put(object, table); + for (var i = 0; i < objects.length; i++) table.rawset(i + 1, toValue(objects[i], values)); return table; } - return wrapLuaObject(object); + var table = new LuaTable(); + if (makeLuaObject(object, table)) { + values.put(object, table); + return table; + } + + LOG.warn(Logging.JAVA_ERROR, "Received unknown type '{}', returning nil.", object.getClass().getName()); + return Constants.NIL; } Varargs toValues(@Nullable Object[] objects) throws LuaError { diff --git a/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java b/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java index 0ba8dd1a0..0dd438e6a 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java +++ b/projects/core/src/main/java/dan200/computercraft/core/util/LuaUtil.java @@ -36,7 +36,20 @@ public class LuaUtil { * @param value The value to test. * @return Whether this is a singleton collection. */ - public static boolean isSingletonCollection(Object value) { - return value == EMPTY_LIST || value == EMPTY_SET || value == EMPTY_MAP; + public static boolean isSingletonCollection(Collection value) { + return value == EMPTY_LIST || value == EMPTY_SET; + } + + /** + * Determine whether a value is a singleton map, such as one created with {@link Map#of()}. + *

+ * These collections are treated specially by {@link ILuaMachine} implementations: we skip sharing for them, and + * create a new table each time. + * + * @param value The value to test. + * @return Whether this is a singleton map. + */ + public static boolean isSingletonMap(Map value) { + return value == EMPTY_MAP; } } diff --git a/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua b/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua index 95a50b369..9c5980adf 100644 --- a/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua +++ b/projects/core/src/test/resources/test-rom/spec/apis/os_spec.lua @@ -202,6 +202,14 @@ describe("The os library", function() expect(xs[1]):eq(xs[2]) end) + it("handles recursive tables", function() + local tbl = {} + tbl[1] = tbl + + local xs = roundtrip(tbl) + expect(xs):eq(xs[1]) + end) + it("does not preserve references in separate args", function() -- I'm not sure I like this behaviour, but it is what CC has always done. local tbl = {}