From b145485ea04e03d34bf6d63a860f2e8ab8c57165 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Fri, 5 Aug 2022 19:00:30 +0200 Subject: [PATCH] Don't leak polyglot null values to Enso Fixed a bug in our handling of polyglot values by 1) doing coercion of primitive values on all languages 2) explicitly checking for null values 3) whenever foreign calls return null we translate that to Nothing --- .../epb/node/CoercePrimitiveNode.java | 5 ++ .../interpreter/epb/node/JsForeignNode.java | 7 ++- .../interpreter/epb/node/RForeignNode.java | 4 +- .../interop/generic/ReadArrayElementNode.java | 50 ++++++++++--------- .../foreign/ForeignMethodCallNode.java | 8 ++- test/Tests/src/Data/Vector_Spec.enso | 4 +- test/Tests/src/Semantic/Js_Interop_Spec.enso | 7 +++ .../src/Semantic/Python_Interop_Spec.enso | 4 ++ test/Tests/src/Semantic/R_Interop_Spec.enso | 7 +++ 9 files changed, 66 insertions(+), 30 deletions(-) diff --git a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java index 2025308adc242..5039fc4496c3f 100644 --- a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java +++ b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java @@ -64,6 +64,11 @@ long doInteger(Object integer, @CachedLibrary(limit = "5") InteropLibrary number } } + @Specialization(guards = "interop.isNull(value)") + Object doNull(Object value, @CachedLibrary(limit = "5") InteropLibrary interop) { + return null; + } + @Fallback Object doNonPrimitive(Object value) { return value; diff --git a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/JsForeignNode.java b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/JsForeignNode.java index 1594055eccd66..9b3c63539fb24 100644 --- a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/JsForeignNode.java +++ b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/JsForeignNode.java @@ -12,6 +12,8 @@ @NodeField(name = "arity", type = int.class) public abstract class JsForeignNode extends ForeignFunctionCallNode { + private @Child CoercePrimitiveNode coercePrimitiveNode = CoercePrimitiveNode.build(); + abstract Object getForeignFunction(); abstract int getArity(); @@ -35,8 +37,9 @@ Object doExecute( Object[] positionalArgs = new Object[newLength]; System.arraycopy(arguments, 1, positionalArgs, 0, newLength); try { - return interopLibrary.invokeMember( - getForeignFunction(), "apply", arguments[0], new ReadOnlyArray(positionalArgs)); + return coercePrimitiveNode.execute( + interopLibrary.invokeMember( + getForeignFunction(), "apply", arguments[0], new ReadOnlyArray(positionalArgs))); } catch (UnsupportedMessageException | UnknownIdentifierException | ArityException diff --git a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/RForeignNode.java b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/RForeignNode.java index 8670a8b7385ae..d3703af4b0491 100644 --- a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/RForeignNode.java +++ b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/RForeignNode.java @@ -11,13 +11,15 @@ @NodeField(name = "foreignFunction", type = Object.class) public abstract class RForeignNode extends ForeignFunctionCallNode { + private @Child CoercePrimitiveNode coercePrimitiveNode = CoercePrimitiveNode.build(); + abstract Object getForeignFunction(); @Specialization public Object doExecute( Object[] arguments, @CachedLibrary("foreignFunction") InteropLibrary interopLibrary) { try { - return interopLibrary.execute(getForeignFunction(), arguments); + return coercePrimitiveNode.execute(interopLibrary.execute(getForeignFunction(), arguments)); } catch (UnsupportedMessageException | UnsupportedTypeException | ArityException e) { throw new IllegalStateException("R parser returned a malformed object", e); } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java index b9f94074428b8..1b9f562b38735 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/interop/generic/ReadArrayElementNode.java @@ -13,32 +13,34 @@ import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod( - type = "Polyglot", - name = "read_array_element", - description = "Returns the size of a polyglot array.") + type = "Polyglot", + name = "read_array_element", + description = "Returns the size of a polyglot array.") public class ReadArrayElementNode extends Node { - private @Child - InteropLibrary library = - InteropLibrary.getFactory().createDispatched(Constants.CacheSizes.BUILTIN_INTEROP_DISPATCH); + private @Child InteropLibrary library = + InteropLibrary.getFactory().createDispatched(Constants.CacheSizes.BUILTIN_INTEROP_DISPATCH); - private @Child - CoercePrimitiveNode coercion = CoercePrimitiveNode.build(); - private final BranchProfile err = BranchProfile.create(); + private @Child CoercePrimitiveNode coercion = CoercePrimitiveNode.build(); + private final BranchProfile err = BranchProfile.create(); - Object execute(Object array, long index) { - try { - Object elem = library.readArrayElement(array, index); - return coercion.execute(elem); - } catch (UnsupportedMessageException e) { - err.enter(); - Builtins builtins = Context.get(this).getBuiltins(); - throw new PanicException( - builtins.error().makeTypeError(builtins.array(), array, "array"), this); - } catch (InvalidArrayIndexException e) { - err.enter(); - Builtins builtins = Context.get(this).getBuiltins(); - throw new PanicException( - builtins.error().makeInvalidArrayIndexError(array, index), this); - } + Object execute(Object array, long index) { + try { + Object elem = coercion.execute(library.readArrayElement(array, index)); + if (elem == null) { + Builtins builtins = Context.get(this).getBuiltins(); + return builtins.nothing().newInstance(); + } else { + return elem; + } + } catch (UnsupportedMessageException e) { + err.enter(); + Builtins builtins = Context.get(this).getBuiltins(); + throw new PanicException( + builtins.error().makeTypeError(builtins.array(), array, "array"), this); + } catch (InvalidArrayIndexException e) { + err.enter(); + Builtins builtins = Context.get(this).getBuiltins(); + throw new PanicException(builtins.error().makeInvalidArrayIndexError(array, index), this); } + } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java index dc7b6b65f4a4a..1888c99071d83 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/foreign/ForeignMethodCallNode.java @@ -6,6 +6,7 @@ import com.oracle.truffle.api.nodes.ExplodeLoop; import com.oracle.truffle.api.profiles.BranchProfile; import org.enso.interpreter.node.ExpressionNode; +import org.enso.interpreter.runtime.Context; import org.enso.interpreter.runtime.error.DataflowError; /** Performs a call into a given foreign call target. */ @@ -46,6 +47,11 @@ public Object executeGeneric(VirtualFrame frame) { return args[i]; } } - return callNode.call(args); + Object result = callNode.call(args); + if (result == null) { + return Context.get(this).getBuiltins().nothing().newInstance(); + } else { + return result; + } } } diff --git a/test/Tests/src/Data/Vector_Spec.enso b/test/Tests/src/Data/Vector_Spec.enso index 8fa37e971629b..bba9a23abe6d6 100644 --- a/test/Tests/src/Data/Vector_Spec.enso +++ b/test/Tests/src/Data/Vector_Spec.enso @@ -29,7 +29,7 @@ foreign js generate_nested_js_array = """ return [[1, 2, 3], [4, 5]] foreign python generate_py_array = """ - return [1, 2, 3, 4, 5] + return [1, 2, 3, 4, None] foreign python generate_nested_py_array = """ return [[1, 2, 3], [4, 5]] @@ -49,7 +49,7 @@ spec = Test.group "Vectors" <| built_from_js = Vector.from_polyglot_array generate_js_array built_from_js . should_equal [1, 2, 3, 4, 5] built_from_py = Vector.from_polyglot_array generate_py_array - built_from_py . should_equal [1, 2, 3, 4, 5] + built_from_py . should_equal [1, 2, 3, 4, Nothing] Test.specify "should allow creation from arrays without mutability for nested arrays" pending="Polyglot Arrays/Vector rewrite" <| built_from_js = Vector.from_polyglot_array generate_nested_js_array diff --git a/test/Tests/src/Semantic/Js_Interop_Spec.enso b/test/Tests/src/Semantic/Js_Interop_Spec.enso index aa317b08bd071..39399d0d1239f 100644 --- a/test/Tests/src/Semantic/Js_Interop_Spec.enso +++ b/test/Tests/src/Semantic/Js_Interop_Spec.enso @@ -60,6 +60,9 @@ foreign js make_false = """ foreign js make_double = """ return 10.5 +foreign js make_null = """ + return null; + foreign js does_not_parse = """ return { x @@ -131,6 +134,10 @@ spec = Test.group "Polyglot JS" <| _ -> False r.should_be_true + Test.specify "should make JS null values equal to Nothing" <| + js_null = make_null + js_null . should_equal Nothing + Test.specify "should make JS numbers type pattern-matchable" <| int_match = case make_int of Integer -> True diff --git a/test/Tests/src/Semantic/Python_Interop_Spec.enso b/test/Tests/src/Semantic/Python_Interop_Spec.enso index f3022bb3d7eb7..123bf507fa918 100644 --- a/test/Tests/src/Semantic/Python_Interop_Spec.enso +++ b/test/Tests/src/Semantic/Python_Interop_Spec.enso @@ -140,6 +140,10 @@ spec = Number -> True num_double_match.should_be_true + Test.specify "should make Python None values equal to Nothing" <| + py_null = make_null + py_null . should_equal Nothing + Test.specify "should allow Enso to catch Python exceptions" <| value = My_Type 1 2 result = Panic.recover Any <| value.my_throw diff --git a/test/Tests/src/Semantic/R_Interop_Spec.enso b/test/Tests/src/Semantic/R_Interop_Spec.enso index 76eea7f11e69b..a553a0757be76 100644 --- a/test/Tests/src/Semantic/R_Interop_Spec.enso +++ b/test/Tests/src/Semantic/R_Interop_Spec.enso @@ -50,6 +50,9 @@ foreign r make_true = """ foreign r make_false = """ FALSE +foreign r make_null = """ + NULL + spec = pending = if Polyglot.is_language_installed "R" then Nothing else """ Can't run R tests, R is not installed. @@ -123,6 +126,10 @@ spec = Number -> True num_double_match.should_be_true + Test.specify "should make R null objects equal to Nothing" <| + r_null = make_null + r_null . should_equal Nothing + Test.specify "should allow Enso to catch R exceptions" <| value = My_Type 1 2 result = Panic.recover Any <| value.my_throw