From a0f4942c5d636eb2a6951d331636df2ae7dfd8ab Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Tue, 11 Oct 2016 17:02:32 -0400
Subject: [PATCH] ChangeNotes: Make defensive copies of mutable value types

ChangeNotesState stores immutable collections, but the actual
underlying ReviewDb types are frustratingly mutable. This means that
if a caller accidentally mutates an instance returned from the cache,
the cache can be polluted. In many cases this is mostly harmless,
since shortly after polluting the cache, the entry is rendered
obsolete by a NoteDb update. However, there are still cases where the
pollution can show up, for example if an exception is thrown from
PostReview after mutating an approval score.

Fix this by making more defensive copies in ChangeNotes. As a
compromise to avoid lots and lots of copies, only do this for types
that we expect to be mutable, namely PatchSet and PatchSetApproval.
Also don't worry about deep copying of ID types, which are technically
mutable but the Gerrit code never calls the setters for.

Change-Id: Iea647804b618422e1aade230fea5ebf6845198aa
---
 .../gerrit/reviewdb/client/PatchSet.java       | 10 ++++++++++
 .../reviewdb/client/PatchSetApproval.java      |  4 ++++
 .../gerrit/server/notedb/ChangeNotes.java      | 18 ++++++++++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
index a8bf07b813..2210319ec8 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
@@ -201,6 +201,16 @@ public final class PatchSet {
     id = k;
   }
 
+  public PatchSet(PatchSet src) {
+    this.id = src.id;
+    this.revision = src.revision;
+    this.uploader = src.uploader;
+    this.createdOn = src.createdOn;
+    this.draft = src.draft;
+    this.groups = src.groups;
+    this.pushCertificate = src.pushCertificate;
+  }
+
   public PatchSet.Id getId() {
     return id;
   }
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java
index c87c66cd66..a2becc21f9 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java
@@ -121,6 +121,10 @@ public final class PatchSetApproval {
     tag = src.tag;
   }
 
+  public PatchSetApproval(PatchSetApproval src) {
+    this(src.getPatchSetId(), src);
+  }
+
   public PatchSetApproval.Key getKey() {
     return key;
   }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 360785fdda..edb5b4e803 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -30,6 +30,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Multimaps;
 import com.google.common.collect.Ordering;
@@ -346,6 +347,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   private DraftCommentNotes draftCommentNotes;
   private RobotCommentNotes robotCommentNotes;
 
+  // Lazy defensive copies of mutable ReviewDb types, to avoid polluting the
+  // ChangeNotesCache from handlers.
+  private ImmutableMap<PatchSet.Id, PatchSet> patchSets;
+  private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
+
   @VisibleForTesting
   public ChangeNotes(Args args, Change change) {
     this(args, change, true, null);
@@ -363,11 +369,19 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   }
 
   public ImmutableMap<PatchSet.Id, PatchSet> getPatchSets() {
-    return state.patchSets();
+    if (patchSets == null) {
+      patchSets = ImmutableMap.copyOf(
+          Maps.transformValues(state.patchSets(), PatchSet::new));
+    }
+    return patchSets;
   }
 
   public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {
-    return state.approvals();
+    if (approvals == null) {
+      approvals = ImmutableListMultimap.copyOf(
+          Multimaps.transformValues(state.approvals(), PatchSetApproval::new));
+    }
+    return approvals;
   }
 
   public ReviewerSet getReviewers() {