From 7eb3b691da86bc77079952c127614d038f23ddfd Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Tue, 27 Jun 2023 18:25:34 +0100 Subject: [PATCH] Fix misplaced calls to IArguments.escapes - Fix mainThread=true methods calling IArguments.escapes too late. This should be done before scheduling on the main thread, not on the main thread itself! - Fix VarargsArguments.escapes not checking that the argument haven't been closed. This is slightly prone to race conditions, but I don't think it's worth the overhead of tracking the owning thread. Maybe when panama and its resource scopes are released. Thanks Sara for pointing this out! Slightly irked that none of our tests caught this. Alas. Also fix a typo in AddressPredicate. Yes, no commit discipline. --- .../java/dan200/computercraft/api/lua/IArguments.java | 4 +++- .../core/apis/http/options/AddressPredicate.java | 2 +- .../dan200/computercraft/core/asm/LuaMethodSupplier.java | 5 ++++- .../computercraft/core/asm/PeripheralMethodSupplier.java | 5 ++++- .../dan200/computercraft/core/lua/VarargArguments.java | 1 + .../computercraft/core/lua/VarargArgumentsTest.java | 8 ++++++++ 6 files changed, 21 insertions(+), 4 deletions(-) diff --git a/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java b/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java index 3f3e0f894..336f0b926 100644 --- a/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java +++ b/projects/core-api/src/main/java/dan200/computercraft/api/lua/IArguments.java @@ -479,9 +479,11 @@ default String optString(int index, @Nullable String def) throws LuaException { * yourself. * * @return An {@link IArguments} instance which can escape the current scope. May be {@code this}. - * @throws LuaException For the same reasons as {@link #get(int)}. + * @throws LuaException For the same reasons as {@link #get(int)}. + * @throws IllegalStateException If marking these arguments as escaping outside the scope of the original function. */ default IArguments escapes() throws LuaException { + // TODO(1.21.0): Make this return void, require that it mutates this. return this; } } diff --git a/projects/core/src/main/java/dan200/computercraft/core/apis/http/options/AddressPredicate.java b/projects/core/src/main/java/dan200/computercraft/core/apis/http/options/AddressPredicate.java index ce4760452..25252a518 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/apis/http/options/AddressPredicate.java +++ b/projects/core/src/main/java/dan200/computercraft/core/apis/http/options/AddressPredicate.java @@ -70,7 +70,7 @@ public static HostRange parse(String addressStr, String prefixSizeStr) { } catch (IllegalArgumentException e) { LOG.error( "Malformed http whitelist/blacklist entry '{}': Cannot extract IP address from '{}'.", - addressStr + '/' + prefixSizeStr, prefixSizeStr + addressStr + '/' + prefixSizeStr, addressStr ); return null; } diff --git a/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethodSupplier.java b/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethodSupplier.java index 11f8e4310..7058ed5b5 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethodSupplier.java +++ b/projects/core/src/main/java/dan200/computercraft/core/asm/LuaMethodSupplier.java @@ -21,7 +21,10 @@ */ public final class LuaMethodSupplier { private static final Generator GENERATOR = new Generator<>(LuaMethod.class, List.of(ILuaContext.class), - m -> (target, context, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, args.escapes()))) + m -> (target, context, args) -> { + var escArgs = args.escapes(); + return context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, escArgs))); + } ); private static final IntCache DYNAMIC = new IntCache<>( method -> (instance, context, args) -> ((IDynamicLuaObject) instance).callMethod(context, method, args) diff --git a/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethodSupplier.java b/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethodSupplier.java index 174b4ef19..ca7fa8ff3 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethodSupplier.java +++ b/projects/core/src/main/java/dan200/computercraft/core/asm/PeripheralMethodSupplier.java @@ -22,7 +22,10 @@ */ public class PeripheralMethodSupplier { private static final Generator GENERATOR = new Generator<>(PeripheralMethod.class, List.of(ILuaContext.class, IComputerAccess.class), - m -> (target, context, computer, args) -> context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, computer, args.escapes()))) + m -> (target, context, computer, args) -> { + var escArgs = args.escapes(); + return context.executeMainThreadTask(() -> ResultHelpers.checkNormalResult(m.apply(target, context, computer, escArgs))); + } ); private static final IntCache DYNAMIC = new IntCache<>( method -> (instance, context, computer, args) -> ((IDynamicPeripheral) instance).callMethod(computer, context, method, args) diff --git a/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java b/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java index 4289d2919..51b71de8c 100644 --- a/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java +++ b/projects/core/src/main/java/dan200/computercraft/core/lua/VarargArguments.java @@ -191,6 +191,7 @@ public Optional optBytes(int index) throws LuaException { @Override public IArguments escapes() { if (escapes) return this; + if (isClosed()) throw new IllegalStateException("Cannot call escapes after IArguments has been closed."); var cache = this.cache; var typeNames = this.typeNames; diff --git a/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java b/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java index 7a76ea562..97a6aa36c 100644 --- a/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java +++ b/projects/core/src/test/java/dan200/computercraft/core/lua/VarargArgumentsTest.java @@ -60,4 +60,12 @@ public void testGetAfterClose() { assertThrows(IllegalStateException.class, () -> args.get(0)); assertThrows(IllegalStateException.class, () -> args.getType(0)); } + + @Test + public void testEscapeAfterClose() { + var args = VarargArguments.of(tableWithCustomType()); + args.close(); + + assertThrows(IllegalStateException.class, args::escapes); + } }