SEC-1795: Changing ACL parent causes a NPE #2028

Closed
spring-issuemaster opened this Issue Aug 10, 2011 · 6 comments

1 participant

@spring-issuemaster

Simon van der Sluis (Migrated from SEC-1795) said:

Changing the the parent ACL of an AclImpl when it has a parent already causes a NullpointException in AclImpl.equals(.)

The unit test (paste into AclImplTests.java) proves it.

@Test
public void testChangeParent() throws Exception
{
AclImpl parentAcl = new AclImpl(objectIdentity, 1L, mockAuthzStrategy, mockAuditLogger);
AclImpl childAcl = new AclImpl(objectIdentity, 2L, mockAuthzStrategy, mockAuditLogger);
AclImpl changeParentAcl = new AclImpl(objectIdentity, 3L, mockAuthzStrategy, mockAuditLogger);

// This works
childAcl.setParent(parentAcl);
childAcl.setParent(null);                        // setting to null first avoids NPE
childAcl.setParent(changeParentAcl);

// This causes an NPE
childAcl.setParent(parentAcl);

}

@spring-issuemaster

Simon van der Sluis said:

A fix in AclImpl.equals(.) as shown below fixes it.

public boolean equals(Object obj) {
if (obj instanceof AclImpl) {
AclImpl rhs = (AclImpl) obj;
if (this.aces.equals(rhs.aces)) {
if ((this.parentAcl == null && rhs.parentAcl == null) || (this.parentAcl !=null && this.parentAcl.equals(rhs.parentAcl))) {
if ((this.objectIdentity == null && rhs.objectIdentity == null) || (this.objectIdentity.equals(rhs.objectIdentity))) {
if ((this.id == null && rhs.id == null) || (this.id.equals(rhs.id))) {
if ((this.owner == null && rhs.owner == null) || this.owner.equals(rhs.owner)) {
if (this.entriesInheriting == rhs.entriesInheriting) {
if ((this.loadedSids == null && rhs.loadedSids == null)) {
return true;
}
if (this.loadedSids.size() == rhs.loadedSids.size()) {
for (int i = 0; i < this.loadedSids.size(); i++) {
if (!this.loadedSids.get(i).equals(rhs.loadedSids.get(i))) {
return false;
}
}
return true;
}
}
}
}
}
}
}
}
return false;
}

@spring-issuemaster

Simon van der Sluis said:

I had hoped that wiki markup would work in the above comment, instead the *'s show where the code was modified.

@spring-issuemaster

Simon van der Sluis said:

It's a pretty ugly piece of code for an equals method, would it be better to use an equals builder from apache-commons-lang?

@spring-issuemaster

Simon van der Sluis said:

And since equals(.) is implemented, hashCode() probably should be too.

@spring-issuemaster

Simon van der Sluis said:

If it's not clear from the junit, setting the parent to null before changing it to a new value is a simple workaround for this bug.

@spring-issuemaster

Luke Taylor said:

Thanks. I've fixed that plus some other possible NPE occurrences in the same method.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.RC3 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment