Skip to content

Commit

Permalink
Don't leak polyglot null values to Enso
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hubertp committed Aug 5, 2022
1 parent e02e8a6 commit b145485
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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;
}
}
}
4 changes: 2 additions & 2 deletions test/Tests/src/Data/Vector_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions test/Tests/src/Semantic/Js_Interop_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions test/Tests/src/Semantic/Python_Interop_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions test/Tests/src/Semantic/R_Interop_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b145485

Please sign in to comment.