Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-70080] Do not generate invalid bytecode for field assignments that use compound operators #103

Merged
merged 4 commits into from Mar 20, 2023
Merged

[JENKINS-70080] Do not generate invalid bytecode for field assignments that use compound operators #103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89a23c4
[JENKINS-70080] Do not generate invalid bytecode for field assignment…
dwnusbaum Feb 3, 2023
017bd00
[JENKINS-70080] Simplify code based on review feedback
dwnusbaum Feb 7, 2023
6c8ab91
Merge branch 'master' into JENKINS-70080
dwnusbaum Mar 20, 2023
b3af9f7
Add comments explaining why we avoid some superclass methods in Scope…
dwnusbaum Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java
View file
Open in desktop
Expand Up @@ -683,8 +683,21 @@ private Expression innerTransform(Expression exp) {
// "this.x = y" must be handled specially to prevent the sandbox from using
// reflection to assign values to final fields in constructors and initializers
// and to prevent infinite loops in setter methods.
return new BinaryExpression(new FieldExpression(field), be.getOperation(),
Token op = be.getOperation();
if (be.getOperation().getType() == Types.ASSIGN) {
dwnusbaum marked this conversation as resolved.
Show resolved Hide resolved
return new BinaryExpression(new FieldExpression(field), op,
makeCheckedGroovyCast(field.getType(), transform(be.getRightExpression())));
} else {
// Groovy does not support FieldExpression with compound assignment operators
// directly, so we must expand the expression ourselves.
Token plainAssignment = Token.newSymbol(Types.ASSIGN, op.getStartLine(), op.getStartColumn());
return new BinaryExpression(new FieldExpression(field), plainAssignment,
makeCheckedGroovyCast(field.getType(),
makeCheckedCall("checkedBinaryOp",
new FieldExpression(field),
intExp(Ops.compoundAssignmentToBinaryOperator(op.getType())),
transform(be.getRightExpression()))));
}
} // else this is a property which we need to check
}
if (interceptProperty)
Expand Down
89 changes: 89 additions & 0 deletions src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java
View file
Open in desktop
Expand Up @@ -1131,4 +1131,93 @@ public void sandboxSupportsReturnStatementsInClosures() throws Exception {
"Script2.result=ArrayList",
"Script2.result");
}

@Test
public void sandboxSupportsFinalFields() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These first two tests are just regression tests that demonstrate why we need this whole special case in the first place.

assertIntercept(
"class Test {\n" +
" final String p\n" +
" Test() {\n" +
" p = 'value'\n" +
" }\n" +
"}\n" +
"new Test().p",
"value",
// Intercepted operations:
"new Test()",
"Test.p");
}

@Test
public void sandboxDoesNotRecurseInfinitelyInSetters() {
assertIntercept(
"class Test {\n" +
" String prop\n" +
" def setProp(newProp) {\n" +
" prop = newProp\n" +
" prop\n" +
" }\n" +
"}\n" +
"new Test().prop = 'value'",
"value",
// Intercepted operations:
"new Test()",
"Test.prop=String",
"Test.prop");
}

@Test
public void sandboxDoesNotPerformImplicitCastsForOverloadedOperators() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added this because we did not already have any tests that explicitly covered this scenario.

// groovy.lang.MissingMethodException: No signature of method: OverridePlus.plus() is applicable for argument types: (java.util.ArrayList) values: [[secret.key]]
assertFailsWithSameException(
"class OverridePlus {\n" +
" def file\n" +
" OverridePlus plus(File file) {\n" +
" this.file = file\n" +
" this\n" +
" }\n" +
"}\n" +
"new OverridePlus() + ['secret.key']\n");
}

@Issue("JENKINS-70080")
@Test
public void sandboxSupportsCompoundAssignmentsToFields() throws Throwable {
assertIntercept(
"class Test {\n" +
" def map = [:]\n" +
" def add(newMap) {\n" +
" map += newMap\n" +
" map\n" +
" }\n" +
"}\n" +
"new Test().add([k: 'v'])\n",
Collections.singletonMap("k", "v"),
// Intercepted operations:
"new Test()",
"Test.add(LinkedHashMap)",
"LinkedHashMap.plus(LinkedHashMap)",
"Test.map");
assertIntercept(
"class Test {\n" +
" final Map map = [:]\n" +
" Test(newMap) {\n" +
" map += newMap\n" + // Groovy is more lenient with 'final' than Java
" }\n" +
"}\n" +
"new Test([k: 'v']).map\n",
Collections.singletonMap("k", "v"),
// Intercepted operations:
"new Test(LinkedHashMap)",
"LinkedHashMap.plus(LinkedHashMap)",
"Test.map");
assertFailsWithSameException(
"class Test {\n" +
" final Map map\n" +
" Test(newMap) {\n" +
" map += newMap\n" + // java.lang.NullPointerException: Cannot execute null+{}
" }\n" +
"}\n" +
"new Test([:]).map\n");
}
}