From: Tom Pantelis <tpanteli@brocade.com>
Date: Mon, 6 Mar 2017 13:44:41 +0000 (-0500)
Subject: Bug 7901: Prevent null Modification in BatchedModifications
X-Git-Tag: release/carbon~181
X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=8f2b69d0944fe8ebc7e4300bb7a05265c7c290d7

Bug 7901: Prevent null Modification in BatchedModifications

Caused by: java.lang.NullPointerException
   at org.opendaylight.controller.cluster.datastore.modification.
      MutableCompositeModification.writeExternal(MutableCompositeModification.java:120)

Somehow a null Modification instance got added to the modifications List.
Looking at the callers of addModification, it is not clear which call site
might have been the culprit. So I added a guard in addModification against
a null input Modification and logged an error with stack trace. I also
modified getModifications to return an immutable List to prevent callers
from directly modifying the List (there was one).

Change-Id: Ic63aa9daada0548da05fe663a0d22cdcb3e7bceb
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
---

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
index 6334bdcd07..ba64e29d75 100644
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
@@ -139,13 +139,15 @@ public class RemoteTransactionContext extends AbstractTransactionContext {
             batchedModifications.setReady(ready);
             batchedModifications.setDoCommitOnReady(doCommitOnReady);
             batchedModifications.setTotalMessagesSent(++totalBatchedModificationsSent);
-            sent = executeOperationAsync(batchedModifications, actorContext.getTransactionCommitOperationTimeout());
 
+            final BatchedModifications toSend = batchedModifications;
             if (ready) {
                 batchedModifications = null;
             } else {
                 batchedModifications = newBatchedModifications();
             }
+
+            sent = executeOperationAsync(toSend, actorContext.getTransactionCommitOperationTimeout());
         }
 
         return sent;
diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
index 9c65ed71d2..a695829744 100644
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
@@ -266,7 +266,7 @@ class EntityOwnershipShardCommitCoordinator {
 
     private void newInflightCommitWithDifferentTransactionID() {
         BatchedModifications newBatchedModifications = newBatchedModifications();
-        newBatchedModifications.getModifications().addAll(inflightCommit.getModifications());
+        newBatchedModifications.addModifications(inflightCommit.getModifications());
         inflightCommit = newBatchedModifications;
     }
 
diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java
index 2bc6d1d84c..ced960db5c 100644
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java
@@ -21,7 +21,7 @@ public interface CompositeModification extends Modification {
     /**
      * Get a list of modifications contained by this composite.
      *
-     * @return list of modifications
+     * @return an immutable list of modifications
      */
     List<Modification> getModifications();
 }
diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java
index 6372b21bef..f2a8c00d19 100644
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java
@@ -13,6 +13,7 @@ import java.io.IOException;
 import java.io.ObjectInput;
 import java.io.ObjectOutput;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import org.opendaylight.controller.cluster.datastore.DataStoreVersions;
 import org.opendaylight.controller.cluster.datastore.messages.VersionedExternalizableMessage;
@@ -29,6 +30,7 @@ public class MutableCompositeModification extends VersionedExternalizableMessage
     private static final long serialVersionUID = 1L;
 
     private final List<Modification> modifications = new ArrayList<>();
+    private List<Modification> immutableModifications = null;
 
     public MutableCompositeModification() {
         this(DataStoreVersions.CURRENT_VERSION);
@@ -63,12 +65,23 @@ public class MutableCompositeModification extends VersionedExternalizableMessage
      * @param modification the modification to add.
      */
     public void addModification(Modification modification) {
+        Preconditions.checkNotNull(modification);
         modifications.add(modification);
     }
 
+    public void addModifications(Iterable<Modification> newMods) {
+        for (Modification mod : newMods) {
+            addModification(mod);
+        }
+    }
+
     @Override
     public List<Modification> getModifications() {
-        return modifications;
+        if (immutableModifications == null) {
+            immutableModifications = Collections.unmodifiableList(modifications);
+        }
+
+        return immutableModifications;
     }
 
     @Override