opendev/gerrit
Code Issues Proposed changes

ChangeNotes: Make defensive copies of mutable value types

Browse Source 
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
This commit is contained in:
Dave Borowitz 2016-10-11 17:02:32 -04:00
parent bb1fc85891
commit a0f4942c5d
3 changed files with 30 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
View File

@ -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;
}

4
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java
View File

@ -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;
}

18
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@ -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() {