JoegenUSTC commented on PR #11755:
URL: https://github.com/apache/gravitino/pull/11755#issuecomment-4786781425
> I think the core issue is not just whether the TCCL is set correctly. The
bigger boundary violation is that CatalogWrapper returns a Capability object
created by the isolated classloader and lets callers invoke it outside the
wrapper/classloader boundary.
>
> A safer fix would be to avoid letting plugin-loaded executable objects
escape from CatalogWrapper. Instead of returning Capability, CatalogWrapper
should expose a doWithCapability(...) method and execute all Capability method
calls inside classLoader.withClassLoader(...). Callers should only receive safe
values/results, not the plugin Capability instance itself.
>
> On the other hand, the format of this PR does not align with our style
guidelines. Please update it accordingly.
Thanks for the thorough review, @diqiu50.
**On the format:** I've updated the PR description to follow the standard
template (`What changes were proposed`, `Why are the changes needed`,
`Does this PR introduce any user-facing change`, `How was this patch
tested`),
and trimmed the title to remove the implementation detail.
Let me know if anything still needs adjustment.
**On the architectural concern:** You're right — the idiomatic pattern in
`CatalogWrapper` is `doWithXxxOps(fn)`, which keeps plugin-loaded objects
inside the classloader boundary and lets callers receive only safe results.
The current `capabilities()` method breaks that contract by returning a live
plugin instance that can be invoked outside `withClassLoader()`.
The proxy in this PR patches the symptom (any invocation on the escaped
instance
is still guarded by TCCL), but it does not fix the boundary violation itself.
A `doWithCapabilityOps(fn)` method would be the architecturally correct
solution.
I see two paths:
1. **Pivot this PR** to implement
`doWithCapabilityOps(ThrowableFunction<Capability, R> fn)`
on `CatalogWrapper`, and refactor all ~40 `getCapability()` call sites
across
the 9 `NormalizeDispatcher` classes. This is the clean fix, but it's a
broader change.
2. **Keep this PR as-is** (minimal TCCL hardening for patterns like
`ServiceLoader` /
`Class.forName`), and track the `doWithCapabilityOps` boundary refactor
as a
follow-up under #11726.
I'm happy to go with option 1 if that's what you prefer — just want to align
on
direction before doing the larger refactor.
One additional note from @yuqi1129's earlier analysis: the `$1`
synthetic-class
`NoClassDefFoundError` we originally set out to fix is actually caused by a
**closed classloader** (cache eviction closes `IsolatedClassLoader` while a
`Capability` instance is still referenced externally), not by a missing
`withClassLoader()` guard. The proxy does not fix that scenario either.
I've updated the PR description accordingly — changed `Fix: #11726` to
`Related: #11726` — since the root CNFE requires the classloader lifecycle
work tracked in #11726, not this change.
--
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]
