You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/06/27 11:40:42 UTC
[PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
paulk-asert opened a new pull request, #2634:
URL: https://github.com/apache/groovy/pull/2634
…eference cycle
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2634:
URL: https://github.com/apache/groovy/pull/2634#issuecomment-4817977527
## ✅ All tests passed ✅
🏷️ Commit: d7d7902b0d6f9cb7a646aff6296a81b1b120d2d5
▶️ Tests: 66740 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2634:
URL: https://github.com/apache/groovy/pull/2634#issuecomment-4817379222
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2634?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `89.65517%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 68.5103%. Comparing base ([`568bf98`](https://app.codecov.io/gh/apache/groovy/commit/568bf980dfd617c3e98d62b51ce38d74e9ae3490?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`facf4d5`](https://app.codecov.io/gh/apache/groovy/commit/facf4d5bc32e51c8ed1de331545179c0844440d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2634?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [src/main/java/groovy/lang/Closure.java](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FClosure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9DbG9zdXJlLmphdmE=) | 89.6552% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2634 +/- ##
==================================================
- Coverage 68.5180% 68.5103% -0.0077%
- Complexity 33597 33599 +2
==================================================
Files 1519 1519
Lines 127384 127413 +29
Branches 23123 23130 +7
==================================================
+ Hits 87281 87291 +10
- Misses 32355 32365 +10
- Partials 7748 7757 +9
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2634?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ava/org/codehaus/groovy/runtime/MethodClosure.java](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FMethodClosure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvTWV0aG9kQ2xvc3VyZS5qYXZh) | `80.7692% <ø> (ø)` | |
| [src/main/java/groovy/lang/Closure.java](https://app.codecov.io/gh/apache/groovy/pull/2634?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FClosure.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9DbG9zdXJlLmphdmE=) | `79.2857% <89.6552%> (+1.1981%)` | :arrow_up: |
... and [6 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2634/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun merged PR #2634:
URL: https://github.com/apache/groovy/pull/2634
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
daniellansun commented on PR #2634:
URL: https://github.com/apache/groovy/pull/2634#issuecomment-4824185696
Merged. Thanks.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12109: Harden Closure deserialization against owner/delegate r… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2634:
URL: https://github.com/apache/groovy/pull/2634#issuecomment-4817472755
## ✅ All tests passed ✅
🏷️ Commit: facf4d5bc32e51c8ed1de331545179c0844440d9
▶️ Tests: 101946 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org