NPE in solveNamingConflict(ModifyDNOperation) when both the moved entry and its new parent are gone

[vharseko]

solveNamingConflict(ModifyDNOperation) dereferences currentDN in the branch which handles a missing new parent, seven lines above the check which says currentDN can be null.

The code

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java

// get the current DN of this entry in the database.
DN currentDN = findEntryDN(entryUUID);                    // :3507, null when the entry is gone
...
if (newSuperiorID != null)
{
  newSuperior = findEntryDN(newSuperiorID);               // :3520, null when the parent is gone
}
else
{
  newSuperior = entryDN.parent();
}

if (newSuperior == null)                                  // :3528
{
  markConflictEntry(op, currentDN, currentDN.parent().child(newRDN));   // :3530  <-- NPE
  numUnresolvedNamingConflicts.incrementAndGet();
  return ConflictResolution.NOTHING_TO_DO;
}

DN newDN = newSuperior.child(newRDN);

if (currentDN == null)                                    // :3537
{
  // The entry targeted by the Modify DN is not in the database anymore.
  // This is a conflict between a delete and this modify DN.
  // The entry has been deleted, we can safely assume that the operation is completed.
  numResolvedNamingConflicts.incrementAndGet();
  return ConflictResolution.NOTHING_TO_DO;
}

findEntryDN() says so itself (:3299): "@return The current DN of the entry or null if there is no entry with the specified UUID". The check at :3537 is there because the author knew it, and :3530 runs first.

When it fires

A replayed ModifyDN carrying a newSuperior, where this replica has since deleted both the entry being moved and the entry it is being moved under: findEntryDN(entryUUID) returns null and findEntryDN(newSuperiorID) returns null, so currentDN.parent() throws. The newSuperiorID == null form of the same branch needs entryDN.parent() to be null, which only a base entry gives, so the two-deletions case is the reachable one.

What it costs

The NPE does not escape: replay() catches it at :2710 with op != null, logs ERR_EXCEPTION_REPLAYING_OPERATION and leaves the change out of the ServerState - correct as far as it goes, and #892 keeps it out of the ServerState rather than recording it as replayed. But nothing about the replica changes between attempts, so every redelivery throws in the same place: the change burns the whole give-up budget and ends in the unreplayed-change alert, for a case the code at :3537 was written to answer in one step - the entry was deleted here, the ModifyDN is resolved, nothing to do.

The markConflictEntry() call is meaningless in that state anyway: it runs an internal modify against currentDN to put ds-sync-conflict on the entry, and there is no entry there to mark (:3766).

What is worth doing

Move the currentDN == null check above the newSuperior == null branch. An entry which is not in the database can not be marked as conflicting and does not need to be: the delete has already settled what the ModifyDN was trying to do, which is exactly what the comment at :3537 says. newDN is only built after the newSuperior == null branch, so nothing in that check depends on the code between them.

Found in a review of the #939 branch. Unrelated to it - #939 is a test for isServerFailure() and does not touch this method.