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 <ismo.haataja@digia.com>
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@theqtcompany.com>

1 files 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);