From 49c544fc51f22342fa64c470fa2cdd6c531bae99 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 6 Sep 2013 12:54:07 -0700 Subject: Fix potential race in MergeOp caused by two reads updateRef() reads the reference and stores the current value into the returned RefUpdate object. This is used to seed the branchTip variable, which is the parent for any commits created by MergeOp. getRef() can read the same reference again and return a more recent SHA-1 as the current value. This may occur if the reference was updated between these two method calls. Replacing the expected old object id here is incorrect. If the branch has advanced MergeOp is now going to build the new commits on top of an older commit, but MergeOp has asked to replace the more recent tip with something else. Change-Id: I64b967c9f044cbd02d5bff5046ed0619a3e08974 (cherry picked from commit 9aa9d989c58e6f305a0913c4f75604e309803c5e) Reviewed-by: Ismo Haataja Reviewed-by: Oswald Buddenhagen --- .../java/com/google/gerrit/server/git/MergeOp.java | 24 ++++++---------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 8db30ec291..020899243f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -419,27 +419,15 @@ public class MergeOp { if (branchUpdate.getOldObjectId() != null) { branchTip = (CodeReviewCommit) rw.parseCommit(branchUpdate.getOldObjectId()); - } else { + } else if (repo.getFullBranch().equals(destBranch.get())) { branchTip = null; - } - - try { - final Ref destRef = repo.getRef(destBranch.get()); - if (destRef != null) { - branchUpdate.setExpectedOldObjectId(destRef.getObjectId()); - } else if (repo.getFullBranch().equals(destBranch.get())) { - branchUpdate.setExpectedOldObjectId(ObjectId.zeroId()); - } else { - for (final Change c : db.changes().submitted(destBranch).toList()) { - setNew(c, message(c, "Your change could not be merged, " - + "because the destination branch does not exist anymore.")); - } + branchUpdate.setExpectedOldObjectId(ObjectId.zeroId()); + } else { + for (final Change c : db.changes().submitted(destBranch).toList()) { + setNew(c, message(c, "Your change could not be merged, " + + "because the destination branch does not exist anymore.")); } - } catch (IOException e) { - throw new MergeException( - "Failed to check existence of destination branch", e); } - return branchUpdate; } catch (IOException e) { throw new MergeException("Cannot open branch", e); -- cgit v1.2.3