Copilot commented on code in PR #2634:
URL: https://github.com/apache/groovy/pull/2634#discussion_r3485966147
##########
src/main/java/groovy/lang/Closure.java:
##########
@@ -1293,6 +1300,74 @@ public Closure<V> rehydrate(Object delegate, Object
owner, Object thisObject) {
return result;
}
+ /**
+ * Deserialization hook that rejects a closure whose {@code owner}/{@code
delegate}/{@code thisObject}
+ * references form a cycle through other closures. Such a cycle cannot
arise from normal Groovy code
+ * or from the {@link #dehydrate()}/{@link #rehydrate} round-trip, but it
can be forged in a
+ * hand-crafted serialized stream; invoking the resulting closure would
then recurse indefinitely and
+ * exhaust the stack (a denial-of-service "gadget").
+ * <p>
+ * This is implemented as {@code readResolve} rather than {@code
readObject} on purpose: a custom
+ * {@code readObject} would interpose this (core-loaded) class on the
stack while the closure's fields
+ * are being read, shifting {@link java.io.ObjectInputStream}'s
latest-user-defined-loader away from the
+ * loader that defined the closure's captured types and breaking
otherwise-valid deserialization.
+ * {@code readResolve} runs after the fields have been read by the default
mechanism, so it does not
+ * affect class resolution.
+ *
+ * @return this closure, unchanged, once validated
+ */
+ @Serial
+ protected Object readResolve() throws ObjectStreamException {
+ // Iterative depth-first search over the raw owner/delegate/thisObject
fields, following only
+ // Closure-valued links. "grey" = on the current DFS path, "black" =
fully explored. An edge back
+ // to a grey node is a genuine cycle; an edge to a black node is a
harmless shared reference (e.g.
+ // a curried closure whose owner and delegate point at the same
wrapped closure). The raw fields
+ // are read directly (not via the overridable getters, which would
themselves recurse on a cyclic
+ // graph); access is permitted as this is the declaring class.
+ final Map<Closure<?>, Boolean> grey = new IdentityHashMap<>();
+ final Set<Closure<?>> black = Collections.newSetFromMap(new
IdentityHashMap<>());
+ final Deque<Object> stack = new ArrayDeque<>();
Review Comment:
In readResolve(), the "grey" structure is used only as a key-set (values are
never read). Using an identity-backed Set like the "black" set would simplify
the code and avoid the unused Boolean values.
##########
src/main/java/groovy/lang/Closure.java:
##########
@@ -1293,6 +1300,74 @@ public Closure<V> rehydrate(Object delegate, Object
owner, Object thisObject) {
return result;
}
+ /**
+ * Deserialization hook that rejects a closure whose {@code owner}/{@code
delegate}/{@code thisObject}
+ * references form a cycle through other closures. Such a cycle cannot
arise from normal Groovy code
+ * or from the {@link #dehydrate()}/{@link #rehydrate} round-trip, but it
can be forged in a
+ * hand-crafted serialized stream; invoking the resulting closure would
then recurse indefinitely and
+ * exhaust the stack (a denial-of-service "gadget").
+ * <p>
+ * This is implemented as {@code readResolve} rather than {@code
readObject} on purpose: a custom
+ * {@code readObject} would interpose this (core-loaded) class on the
stack while the closure's fields
+ * are being read, shifting {@link java.io.ObjectInputStream}'s
latest-user-defined-loader away from the
+ * loader that defined the closure's captured types and breaking
otherwise-valid deserialization.
+ * {@code readResolve} runs after the fields have been read by the default
mechanism, so it does not
+ * affect class resolution.
+ *
+ * @return this closure, unchanged, once validated
+ */
+ @Serial
+ protected Object readResolve() throws ObjectStreamException {
+ // Iterative depth-first search over the raw owner/delegate/thisObject
fields, following only
Review Comment:
Making Closure.readResolve() protected introduces a new overridable method
on a widely-subclassed public type; any existing subclass that already declares
a private readResolve() (a common Java-serialization pattern) will now fail to
compile due to reduced visibility. Consider keeping the serialization hook
private in Closure and delegating the validation logic to a separate
protected/package-private helper for Groovy-internal subclasses that need to
participate.
--
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]
