jdaugherty commented on code in PR #16482:
URL: https://github.com/apache/grails-core/pull/16482#discussion_r4166817508
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java:
##########
@@ -138,11 +143,20 @@
* end of every path. A {@code catch} block starts from the merge of every
state its {@code try}
* block passed through, since an exception may leave the block after any
statement, and a
* {@code finally} block is checked against every path into it. A loop body or
closure body may
- * run any number of times, so it is re-walked from the merged loop-head state
until that state
- * is stable before findings are reported: an assignment late in the body is
seen by a use
- * earlier in it, which the next iteration reaches. {@code break}, {@code
continue} and, in a
- * closure, {@code return} carry their state to the exit or head they jump to,
and a path ending
- * in such a jump or in {@code throw} contributes nothing to the state after
the statement.
+ * run any number of times, so a body that assigns a local declared outside it
is re-walked from
+ * the merged loop-head state until that state is stable before findings are
reported: an
+ * assignment late in the body is seen by a use earlier in it, which the next
iteration reaches.
+ * {@code break}, {@code continue} and, in a closure, {@code return} carry
their state to the
+ * exit or head they jump to, and a path ending in such a jump or in {@code
throw} contributes
Review Comment:
A `break` or `continue` inside a `try` that has a `finally` takes the state
from the moment of the jump to its target. But the `finally` block runs before
the jump lands, so anything it assigns is lost:
```groovy
static void sorted(String sort, List<String> sorts) {
String frag = ' title'
for (String s in sorts) {
try {
if (s == 'stop') break
} finally {
frag = sort
}
frag = ' id'
}
String q = "from Book order by ${frag}"
executeQuery(q) // compiles cleanly; with sorts == ['stop'] it runs
as "from Book order by <sort>"
}
```
The same happens with a `continue` when the query is at the top of the loop
body, and with a `break` out of a `switch` case. All three compile cleanly on
this branch and on `8.0.x`, and fail before #16463. I ran each one to confirm
that the query really contains `sort`. The gap comes from #16463, not from this
change. It is the same kind of false negative as the cases in #16481, though,
so could it be closed here as well?
One way to do it: give each jump made inside the `try` or a `catch` the
state the `finally` block ends with, instead of the state at the jump. The walk
from `intoFinally` already starts from a merge of every jump's state. So
merging its end state into the exit, or head, of each target jumped to from
inside would be enough, and it errs on the safe side.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java:
##########
@@ -795,46 +917,239 @@ void reset() {
}
/**
- * Records what {@code variableName} now holds after being assigned {@code
rightExpression},
+ * An assignment to a local found by the pre-scan: the value assigned, or
{@code null} for an
+ * assignment that is not string building this check understands.
+ */
+ private static final class Write {
+
+ final Object key;
+ final Expression value;
+
+ Write(Object key, Expression value) {
+ this.key = key;
+ this.value = value;
+ }
+ }
+
+ /**
+ * What a pre-scan of the code about to be walked finds: the facts about
its locals that do not
+ * depend on the order its statements run in.
+ */
+ private final class CodeFacts extends CodeVisitorSupport {
+
+ /** Every local declared in the code. */
+ final Set<Object> declared = new HashSet<>();
+ /** Every assignment to a local, declarations with a value included. */
+ final List<Write> writes = new ArrayList<>();
+ /** The locals whose declaration carries the suppression. */
+ final Set<Object> suppressed = new HashSet<>();
+ /**
+ * The locals assigned where the walk cannot place the assignment in
order: inside another
+ * expression (a ternary or Elvis branch, an {@code &&} or {@code ||}
operand, a method
+ * argument, a loop condition), by multiple assignment, or inside a
closure that does not
+ * declare them, which may run at any later point.
+ */
+ final Set<Object> unordered = new HashSet<>();
+ /** The locals and parameters each closure declares. */
+ final Map<ClosureExpression, Set<Object>> closureLocals = new
IdentityHashMap<>();
+ /**
+ * The locals that hold only constant text however their assignments
are ordered, filled
+ * in by {@link #settleConstants} once the scan is complete.
+ */
+ final Set<Object> constant = new HashSet<>();
+
+ /** The loop and closure bodies that assign a local declared outside
them. */
+ final Set<Statement> bodiesAssigningOuterLocals =
Collections.newSetFromMap(new IdentityHashMap<>());
+
+ private final Set<Expression> statementExpressions =
Collections.newSetFromMap(new IdentityHashMap<>());
+ private final Deque<Set<Object>> closureScopes = new ArrayDeque<>();
+ /** The loop and closure bodies being scanned, innermost first. */
+ private final Deque<BodyScope> openBodies = new ArrayDeque<>();
+
+ @Override
+ public void visitExpressionStatement(ExpressionStatement statement) {
+ statementExpressions.add(statement.getExpression());
+ super.visitExpressionStatement(statement);
+ }
+
+ @Override
+ public void visitClosureExpression(ClosureExpression expression) {
Review Comment:
A closure that assigns a local declared outside it is now covered, but an
anonymous inner class that does the same is not. The pre-scan does not descend
into the class body. The class itself is analysed separately, with its own
state, so the enclosing method never learns of the assignment:
```groovy
static void sorted(String sort) {
String frag = ' title'
Runnable r = new Runnable() { void run() { frag = sort } }
r.run()
String q = "from Book order by ${frag}"
executeQuery(q) // compiles cleanly; runs as "from Book order by
<sort>"
}
```
This compiles cleanly on this branch and on `8.0.x`, and fails before
#16463. A Java-style lambda doing the same assignment is already caught,
because it is a `ClosureExpression`. Could the pre-scan treat the method bodies
of an anonymous inner class
(`ConstructorCallExpression.isUsingAnonymousInnerClass()`) like a closure body?
A local of the enclosing method assigned there would then count as unordered.
##########
grails-doc/src/en/guide/security/securingAgainstAttacks.adoc:
##########
@@ -118,7 +118,7 @@ Book.executeQuery("from Book b where 1 = 1 ${restriction}",
[title: params.title
// runs as: from Book b where 1 = 1 :p0 -- the fragment is bound as a value
----
-The check tells query text and values apart. An interpolation that is
*constant text* cannot carry user input, so it is not flagged even when the
`GString` is coerced to a `String`. Constant text is a string literal, a
`static final` field initialized from constant text, a ternary or Elvis
expression choosing between constant text, a `+` concatenation or `GString`
made only of constant text, and a local variable that only ever held constant
text on every path leading to the query, whichever `if`/`else`, `switch`,
`try`/`catch`, loop or closure those paths run through. This is what lets
restrictions be chosen at runtime while the values stay bound:
+The check tells query text and values apart. An interpolation that is
*constant text* cannot carry user input, so it is not flagged even when the
`GString` is coerced to a `String`. Constant text is a string literal, a
`static final` field initialized from constant text, a ternary or Elvis
expression choosing between constant text, a `+` concatenation or `GString`
made only of constant text, and a local variable that only ever held constant
text on every path leading to the query, whichever `if`/`else`, `switch`,
`try`/`catch` or loop those paths run through. A closure can run at any point
after it is defined, and an assignment inside another expression — a ternary or
Elvis branch, an `&&` or `||` operand, a method argument or a loop condition —
or a multiple assignment may or may not run, so a local that a closure reads or
assigns, or that is assigned in one of those ways, counts as constant text only
if every assignment to it in the method is constant text. This is what lets r
estrictions be chosen at runtime while the values stay bound:
Review Comment:
Nit: "a local that a closure reads or assigns" also covers a local declared
inside the closure, which is still tracked in order. The Javadoc wording is
more precise: "a local declared outside a closure that the closure reads or
assigns".
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformerSpec.groovy:
##########
@@ -462,6 +467,156 @@ class Book {
'a loop iteration that returns before reaching the query' |
'String q = "from Book b"\n for (String t in titles) {\n if
(t == title) {\n q = "from Book b where b.title = ${t}"\n
return [q]\n }\n }'
}
+ @Unroll
+ void "test a use inside #construct sees data assigned where the walk
cannot place it"() {
+ when:
+ new GroovyClassLoader().parseClass("""
+import grails.gorm.annotation.Entity
+
+@Entity
+class Book {
+ String title
+
+ static void sorted(String sort, List<String> sorts) {
+ String frag = " title"
+ $body
+ }
+}
+""")
+
+ then:
+ def e = thrown(MultipleCompilationErrorsException)
+ e.message.contains('GormUnsafeQueryString')
+ e.message.contains("passed to 'executeQuery'")
+
+ where:
+ construct | body
+ 'a do/while body assigned in its condition' | 'do {\n
String q = "from Book order by ${frag}"\n executeQuery(q)\n }
while ((frag = sorts.remove(0)) != null)'
+ 'a closure called after a reassignment' | 'Closure run = {\n
String q = "from Book order by ${frag}"\n executeQuery(q)\n
}\n frag = sort\n run()'
+ }
+
+ @Unroll
+ void "test #construct reusing the name of an earlier constant local is not
treated as constant text"() {
+ when:
+ new GroovyClassLoader().parseClass("""
+import grails.gorm.annotation.Entity
+
+@Entity
+class Book {
+ String title
+
+ static void sorted(String sort, List<String> sorts) {
+ $body
+ }
+}
+""")
+
+ then:
+ def e = thrown(MultipleCompilationErrorsException)
+ e.message.contains('GormUnsafeQueryString')
+ e.message.contains("passed to 'executeQuery'")
+
+ where:
+ construct | body
+ 'a loop variable after a try block' | 'try {\n String
part = " title"\n } finally {\n }\n for (String part in
sorts) {\n String q = "from Book order by ${part}"\n
executeQuery(q)\n }'
+ 'a loop variable after an if/else' | 'if (sort) {\n
String part = " id"\n } else {\n String part = " title"\n
}\n for (String part in sorts) {\n String q = "from Book
order by ${part}"\n executeQuery(q)\n }'
+ 'a closure parameter after an if/else' | 'if (sort) {\n
String part = " id"\n } else {\n String part = " title"\n
}\n sorts.each { String part ->\n String q = "from Book
order by ${part}"\n executeQuery(q)\n }'
+ }
+
+ @Unroll
+ void "test a query flattened inside a closure is still flagged when
#construct"() {
+ when:
+ new GroovyClassLoader().parseClass("""
+import grails.gorm.annotation.Entity
+
+@Entity
+class Book {
+ String title
+
+ static void byTitle(List<String> titles, String title) {
+ String q = "from Book"
+ $body
+ }
+}
+""")
+
+ then:
+ def e = thrown(MultipleCompilationErrorsException)
+ e.message.contains('GormUnsafeQueryString')
+ e.message.contains("passed to 'executeQuery'")
+
+ where:
+ construct | body
+ 'it is used after the closure is called' | 'Closure flatten =
{ q = "from Book where title = ${title}" }\n flatten()\n
executeQuery(q)'
+ 'a later run of the closure reaches the use' | 'titles.each {\n
executeQuery(q)\n q = "from Book where title = ${it}"\n
}'
+ 'the closure returns after flattening it' | 'Closure flatten =
{\n if (title) {\n q = "from Book where title =
${title}"\n return\n }\n q = "from Book"\n
}\n flatten()\n executeQuery(q)'
+ }
+
+ void "test a constant local read inside a closure compiles cleanly with no
warnings"() {
+ when:
+ List<WarningMessage> warnings = compileAndCollectWarnings('''
+import grails.gorm.annotation.Entity
+
+@Entity
+class Book {
+ String title
+
+ static void search(List<String> titles, Map queryParams) {
+ String restriction = ""
+ if (queryParams.title) {
+ restriction = " and b.title = :title"
+ }
+ titles.each { String t ->
+ String q = "from Book b where b.title <> :t ${restriction}"
+ executeQuery(q, queryParams + [t: t])
+ }
+ }
+}
+''')
+
+ then:
+ warnings.empty
+ }
+
+ @Unroll
+ void "test #construct nested #depth deep compiles without the check
slowing it down exponentially"() {
+ given:
+ String open = (1..depth).collect { opening.replace('#', it.toString())
}.join('\n')
+ String close = (1..depth).collect { '}' }.join('\n')
+ String source = """
+import grails.gorm.annotation.Entity
+
+@Entity
+class Book {
+ String title
+
+ static List search(List<String> titles) {
+ String q = "from Book b where 1 = 1"
+ $open
+ $innermost
+ $close
+ executeQuery(q)
+ }
+}
+"""
+
+ when:
+ long started = System.nanoTime()
+ List<WarningMessage> warnings = compileAndCollectWarnings(source)
+ long elapsedMillis = (System.nanoTime() - started).intdiv(1_000_000)
+
+ then: 'every level walked twice would take minutes at this depth'
+ warnings.empty
+ elapsedMillis < 10_000
+
+ where:
+ construct | depth | opening
| innermost
+ 'closures' | 24 | 'titles.each { String t#
->' | 'println(t1)'
+ 'for loops' | 24 | 'for (String t# in titles)
{' | 'println(t1)'
+ 'for loops appending to a query' | 24 | 'for (String t# in titles)
{' | 'q += " and b.title is not null"'
+ 'closures appending to a query' | 24 | 'titles.each { String t#
->' | 'q += " and b.title is not null"'
Review Comment:
Optional, low priority. Nested `finally` blocks still cost exponential time,
because each `finally` is walked twice (silently from the normal exits, then
from `intoFinally` to report), and a `try` nested inside it is walked twice on
each of those walks. Compiling `try { } finally { try { } finally { ... } }`:
| depth | this branch | before #16463 |
|---|---|---|
| 16 | 133 ms | 11 ms |
| 18 | 270 ms | 10 ms |
| 20 | 973 ms | 11 ms |
Nobody writes code like this, so it doesn't matter in practice. Still, when
`silentPasses > 0` and `afterFinally != null`, the walk from `intoFinally`
throws away both its findings and its end state, so it could be skipped there.
That would leave every walk in a silent pass linear. If the fix for jumps
through `finally` ends up using the end state of that walk, skip it only when
the `try` contains no jump. A `nested finally` row here would keep the time
bounded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
