[
https://issues.apache.org/jira/browse/CURATOR-489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16709285#comment-16709285
]
ASF GitHub Bot commented on CURATOR-489:
----------------------------------------
GitHub user joshng opened a pull request:
https://github.com/apache/curator/pull/284
CURATOR-489: CreateBuilderImpl assigns protectedId if doProtected is …
This is a light change to correct a bug when `doProtected` is true: the
existing code sets `CreateBuilderImpl.doProtected = true`, but assigns
`protectedId = null`, resulting in a non-unique protection-token (`_c_null-`)
being applied to the name created in zookeeper. Given that all replicas
coordinating via zookeeper would use the identical token, this could
conceivably lead to incorrect behavior when the protection logic is needed.
While I believe this PR fixes the bug, I'd really suggest a more
comprehensive change to eliminate the `CreateBuilderImpl.doProtected` field
entirely, instead relying upon `protectedId != null` to convey this intention.
However, being unfamiliar with the nuances of the existing implementation, I
didn't want to perform that surgery today. If you think that change seems
preferable, I'd be happy to do that instead.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/joshng/curator CURATOR-489-set-protectedId
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/284.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #284
----
commit 5c27ecc29c3503633462cd95971ceb6a91175404
Author: josh gruenberg <joshgr@...>
Date: 2018-12-04T21:17:15Z
CURATOR-489: CreateBuilderImpl assigns protectedId if doProtected is true
----
> CreateOption.doProtected uses null protectedId when applied via
> AsyncCreateBuilderImpl
> --------------------------------------------------------------------------------------
>
> Key: CURATOR-489
> URL: https://issues.apache.org/jira/browse/CURATOR-489
> Project: Apache Curator
> Issue Type: Bug
> Affects Versions: 4.0.1
> Reporter: josh gruenberg
> Assignee: Jordan Zimmerman
> Priority: Major
>
> When trying to apply "protection" to an async node-creation with
> {{CreateOption.doProtected}}, the protection is undermined because the node
> is created with {{protectionId = null}}.
> The {{AsyncCreateBuilderImpl}} does pass {{doProtected = true}} to the
> {{CreateBuilderImpl}} constructor. However, this constructor (incorrectly?)
> assigns {{protectedId = null}}, resulting in a node-prefix of {{_c_null-}}
> (because {{getProtectedPrefix}} does not validate that the provided
> {{protectedId}} is non-null). This entirely undermines the effectiveness of
> the protection.
> {code:java}
> public CreateBuilderImpl(CuratorFrameworkImpl client, CreateMode
> createMode, Backgrounding backgrounding, boolean createParentsIfNeeded,
> boolean createParentsAsContainers, boolean doProtected, boolean compress,
> boolean setDataIfExists, List<ACL> aclList, Stat storingStat, long ttl)
> {
> this.client = client;
> this.createMode = createMode;
> this.backgrounding = backgrounding;
> this.createParentsIfNeeded = createParentsIfNeeded;
> this.createParentsAsContainers = createParentsAsContainers;
> this.doProtected = doProtected;
> this.compress = compress;
> this.setDataIfExists = setDataIfExists;
> protectedId = null; // *** incorrect if
> doProtected?! ***
> this.acling = new ACLing(client.getAclProvider(), aclList);
> this.storingStat = storingStat;
> this.ttl = ttl;
> }
> {code}
> It looks to me as if that constructor should be assigning {{protectedId}} if
> {{doProtected}} is true.
> I wonder if the {{doProtected}} field should just be eliminated in favor a
> method that checks {{protectedId != null}}, to avoid this redundant encoding
> of intent?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
