vyommani commented on code in PR #1068:
URL: https://github.com/apache/ranger/pull/1068#discussion_r3644541893
##########
security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyAdminCache.java:
##########
@@ -193,11 +193,13 @@ private RangerPolicyAdminWrapper
addOrUpdatePolicyAdmin(RangerPolicyAdminWrapper
}
private RangerPolicyAdmin addPolicyAdmin(ServicePolicies policies,
RangerRoles roles, RangerPolicyEngineOptions options) {
- RangerServiceDef serviceDef = policies.getServiceDef();
- String serviceType = (serviceDef != null) ?
serviceDef.getName() : "";
- RangerPluginContext rangerPluginContext = new RangerPluginContext(new
RangerPluginConfig(serviceType, null, "ranger-admin", null, null, options));
+ synchronized (policies) {
Review Comment:
Thanks for digging into this, Sanket - the synchronized(policies) change
does stop the crash for same-service concurrent rebuilds, confirmed that with a
test. But I don't think this is the right fix, for a few reasons, and I'd like
to hold off merging until we talk it through.
**It locks far more than it needs to.** The actual race is a handful of List
mutations inside ServiceDefUtil.normalize() - a few clear()/addAll() and
removeAll() calls. This patch instead synchronizes the entire
RangerPolicyAdminImpl construction: building every policy evaluator, every
context enricher, every resource trie for the service. Under the concurrent
delegated-admin load that triggers this bug, that means policy engine builds
for the same service go from running in parallel to queuing up one at a time
behind this lock. That's a real latency cost under exactly the load pattern
we're trying to fix, not a hypothetical one.
**It doesn't cover the actual shared object.** `policies` is a per-service
ServicePolicies instance, but what's actually being mutated unsafely is the
RangerServiceDef nested inside it - specifically the tag-service one, which
isn't service-specific. It's the same object referenced from every component
service's ServicePolicies. Two different services rebuilding concurrently
synchronize on two different `policies` objects, so this lock provides no
protection between them at all, even though they can both be calling
normalize() on the identical shared tag serviceDef at the same time.
I didn't want to just argue this, so I wrote three concurrency tests against
addPolicyAdmin() directly (happy to share them): same ServicePolicies instance
across threads, a fresh instance per call wrapping the same RangerServiceDef,
and two different services sharing one tag serviceDef. With this patch applied
and confirmed compiled in, the first two pass - so the lock does work for the
case it's scoped to. The third one fails, reproducing the exact CME/NPE from
the original report, just across two services instead of one. That's not a
hypothetical gap, it's reproducible.
**Proposed fix instead:** give RangerServiceDef (and its superclass
RangerBaseModelObject) proper copy constructors - deep-copying the nested
RangerDataMaskDef/RangerRowFilterDef and their access-type/resource lists, not
just the top-level lists - and have ServiceDefUtil.normalize() copy the
serviceDef before mutating it, instead of mutating the shared cached instance
in place. Once normalize() operates on a private copy, there's no shared
mutable state left for any lock to have to get right, so no lock is needed at
all. I ran all three tests against this version and all three pass, including
the cross-service case.
This also fixes the bug at its actual source rather than at one caller of it
- any other code path that calls normalize() on a shared serviceDef gets the
same protection for free, which this patch doesn't give us.
I am attaching my patch which resolve this CME without any lock. It has some
memory cost but i believe we have to live with that.
[change.patch](https://github.com/user-attachments/files/30342276/change.patch)
--
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]
