Fix atomic lock file creation on NFS

FS_POSIX.createNewFile(File) failed to properly implement atomic file
creation on NFS using the algorithm [1]:
- name of the hard link must be unique to prevent that two processes
  using different NFS clients try to create the same link. This would
  render nlink useless to detect if there was a race.
- the hard link must be retained for the lifetime of the file since we
  don't know when the state of the involved NFS clients will be
  synchronized. This depends on NFS configuration options.

To fix these issues we need to change the signature of createNewFile
which would break API. Hence deprecate the old method
FS.createNewFile(File) and add a new method createNewFileAtomic(File).

The new method returns a LockToken which needs to be retained by the
caller (LockFile) until all involved NFS clients synchronized their
state. Since we don't know when the NFS caches are synchronized we need
to retain the token until the corresponding file is no longer needed.
The LockToken must be closed after the LockFile using it has been
committed or unlocked. On Posix, if core.supportsAtomicCreateNewFile =
false this will delete the hard link which guarded the atomic creation
of the file. When acquiring the lock fails ensure that the hard link is
removed.

[1] https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html
also see file creation flag O_EXCL in
http://man7.org/linux/man-pages/man2/open.2.html

Change-Id: I84fcb16143a5f877e9b08c6ee0ff8fa4ea68a90d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -119,6 +119,7 @@ checkoutUnexpectedResult=Checkout returned unexpected result {0}
 classCastNotA=Not a {0}
 cloneNonEmptyDirectory=Destination path "{0}" already exists and is not an empty directory
 closed=closed
+closeLockTokenFailed=Closing LockToken ''{0}'' failed
 collisionOn=Collision on {0}
 commandRejectedByHook=Rejected by "{0}" hook.\n{1}
 commandWasCalledInTheWrongState=Command {0} was called in the wrong state
@@ -284,6 +285,7 @@ expectedLessThanGot=expected less than ''{0}'', got ''{1}''
 expectedPktLineWithService=expected pkt-line with ''# service=-'', got ''{0}''
 expectedReceivedContentType=expected Content-Type {0}; received Content-Type {1}
 expectedReportForRefNotReceived={0}: expected report for ref {1} not received
+failedAtomicFileCreation=Atomic file creation failed, number of hard links to file {0} was not 2 but {1}"
 failedToDetermineFilterDefinition=An exception occured while determining filter definitions
 failedUpdatingRefs=failed updating refs
 failureDueToOneOfTheFollowing=Failure due to one of the following:
@@ -667,6 +669,7 @@ unknownObjectType2=unknown
 unknownRepositoryFormat=Unknown repository format
 unknownRepositoryFormat2=Unknown repository format "{0}"; expected "0".
 unknownZlibError=Unknown zlib error.
+unlockLockFileFailed=Unlocking LockFile ''{0}'' failed
 unmergedPath=Unmerged path: {0}
 unmergedPaths=Repository contains unmerged paths
 unpackException=Exception while parsing pack stream

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -177,6 +177,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String checkoutUnexpectedResult;
 	/***/ public String classCastNotA;
 	/***/ public String cloneNonEmptyDirectory;
+	/***/ public String closeLockTokenFailed;
 	/***/ public String closed;
 	/***/ public String collisionOn;
 	/***/ public String commandRejectedByHook;
@@ -343,6 +344,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String expectedPktLineWithService;
 	/***/ public String expectedReceivedContentType;
 	/***/ public String expectedReportForRefNotReceived;
+	/***/ public String failedAtomicFileCreation;
 	/***/ public String failedToDetermineFilterDefinition;
 	/***/ public String failedUpdatingRefs;
 	/***/ public String failureDueToOneOfTheFollowing;
@@ -726,6 +728,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String unknownRepositoryFormat;
 	/***/ public String unknownRepositoryFormat2;
 	/***/ public String unknownZlibError;
+	/***/ public String unlockLockFileFailed;
 	/***/ public String unmergedPath;
 	/***/ public String unmergedPaths;
 	/***/ public String unpackException;

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java

@@ -64,7 +64,10 @@ import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FS.LockToken;
 import org.eclipse.jgit.util.FileUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Git style file locking and replacement.
@@ -77,6 +80,7 @@ import org.eclipse.jgit.util.FileUtils;
  * name.
  */
 public class LockFile {
+	private final static Logger LOG = LoggerFactory.getLogger(LockFile.class);
 
 	/**
 	 * Unlock the given file.
@@ -132,6 +136,8 @@ public class LockFile {
 
 	private FileSnapshot commitSnapshot;
 
+	private LockToken token;
+
 	/**
 	 * Create a new lock for any file.
 	 *
@@ -170,7 +176,8 @@ public class LockFile {
 	 */
 	public boolean lock() throws IOException {
 		FileUtils.mkdirs(lck.getParentFile(), true);
-		if (FS.DETECTED.createNewFile(lck)) {
+		token = FS.DETECTED.createNewFileAtomic(lck);
+		if (token.isCreated()) {
 			haveLck = true;
 			try {
 				os = new FileOutputStream(lck);
@@ -178,6 +185,8 @@ public class LockFile {
 				unlock();
 				throw ioe;
 			}
+		} else {
+			closeToken();
 		}
 		return haveLck;
 	}
@@ -458,6 +467,7 @@ public class LockFile {
 		try {
 			FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE);
 			haveLck = false;
+			closeToken();
 			return true;
 		} catch (IOException e) {
 			unlock();
@@ -465,6 +475,13 @@ public class LockFile {
 		}
 	}
 
+	private void closeToken() {
+		if (token != null) {
+			token.close();
+			token = null;
+		}
+	}
+
 	private void saveStatInformation() {
 		if (needSnapshot)
 			commitSnapshot = FileSnapshot.save(lck);
@@ -503,8 +520,9 @@ public class LockFile {
 		if (os != null) {
 			try {
 				os.close();
-			} catch (IOException ioe) {
-				// Ignore this
+			} catch (IOException e) {
+				LOG.error(MessageFormat
+						.format(JGitText.get().unlockLockFileFailed, lck), e);
 			}
 			os = null;
 		}
@@ -514,7 +532,10 @@ public class LockFile {
 			try {
 				FileUtils.delete(lck, FileUtils.RETRY);
 			} catch (IOException e) {
-				// couldn't delete the file even after retry.
+				LOG.error(MessageFormat
+						.format(JGitText.get().unlockLockFileFailed, lck), e);
+			} finally {
+				closeToken();
 			}
 		}
 	}

org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java

@@ -45,6 +45,7 @@ package org.eclipse.jgit.util;
 
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -52,6 +53,8 @@ import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.PrintStream;
 import java.nio.charset.Charset;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.text.MessageFormat;
@@ -59,6 +62,7 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
@@ -800,13 +804,78 @@ public abstract class FS {
 	 *            the file to be created
 	 * @return <code>true</code> if the file was created, <code>false</code> if
 	 *         the file already existed
-	 * @throws IOException
+	 * @throws java.io.IOException
+	 * @deprecated use {@link #createNewFileAtomic(File)} instead
 	 * @since 4.5
 	 */
+	@Deprecated
 	public boolean createNewFile(File path) throws IOException {
 		return path.createNewFile();
 	}
 
+	/**
+	 * A token representing a file created by
+	 * {@link #createNewFileAtomic(File)}. The token must be retained until the
+	 * file has been deleted in order to guarantee that the unique file was
+	 * created atomically. As soon as the file is no longer needed the lock
+	 * token must be closed.
+	 *
+	 * @since 4.7
+	 */
+	public static class LockToken implements Closeable {
+		private boolean isCreated;
+
+		private Optional<Path> link;
+
+		LockToken(boolean isCreated, Optional<Path> link) {
+			this.isCreated = isCreated;
+			this.link = link;
+		}
+
+		/**
+		 * @return {@code true} if the file was created successfully
+		 */
+		public boolean isCreated() {
+			return isCreated;
+		}
+
+		@Override
+		public void close() {
+			if (link.isPresent()) {
+				try {
+					Files.delete(link.get());
+				} catch (IOException e) {
+					LOG.error(MessageFormat.format(JGitText.get().closeLockTokenFailed,
+							this), e);
+				}
+			}
+		}
+
+		@Override
+		public String toString() {
+			return "LockToken [lockCreated=" + isCreated + //$NON-NLS-1$
+					", link=" //$NON-NLS-1$
+					+ (link.isPresent() ? link.get().getFileName() + "]" //$NON-NLS-1$
+							: "<null>]"); //$NON-NLS-1$
+		}
+	}
+
+	/**
+	 * Create a new file. See {@link java.io.File#createNewFile()}. Subclasses
+	 * of this class may take care to provide a safe implementation for this
+	 * even if {@link #supportsAtomicCreateNewFile()} is <code>false</code>
+	 *
+	 * @param path
+	 *            the file to be created
+	 * @return LockToken this token must be closed after the created file was
+	 *         deleted
+	 * @throws IOException
+	 * @since 4.7
+	 */
+	public LockToken createNewFileAtomic(File path) throws IOException {
+		return new LockToken(path.createNewFile(), Optional.empty());
+	}
+
 	/**
 	 * See {@link FileUtils#relativize(String, String)}.
 	 *

org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java

@@ -52,14 +52,19 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.attribute.PosixFilePermission;
+import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
+import java.util.UUID;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.api.errors.JGitInternalException;
 import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
@@ -360,9 +365,12 @@ public class FS_POSIX extends FS {
 	 * multiple clients manage to create the same lock file nlink would be
 	 * greater than 2 showing the error.
 	 *
-	 * @see https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html
+	 * @see "https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html"
+	 *
+	 * @deprecated use {@link FS_POSIX#createNewFileAtomic(File)} instead
 	 * @since 4.5
 	 */
+	@Deprecated
 	public boolean createNewFile(File lock) throws IOException {
 		if (!lock.createNewFile()) {
 			return false;
@@ -395,4 +403,70 @@ public class FS_POSIX extends FS {
 			}
 		}
 	}
+
+	/**
+	 * {@inheritDoc}
+	 * <p>
+	 * An implementation of the File#createNewFile() semantics which can create
+	 * a unique file atomically also on NFS. If the config option
+	 * {@code core.supportsAtomicCreateNewFile = true} (which is the default)
+	 * then simply File#createNewFile() is called.
+	 *
+	 * But if {@code core.supportsAtomicCreateNewFile = false} then after
+	 * successful creation of the lock file a hard link to that lock file is
+	 * created and the attribute nlink of the lock file is checked to be 2. If
+	 * multiple clients manage to create the same lock file nlink would be
+	 * greater than 2 showing the error. The hard link needs to be retained
+	 * until the corresponding file is no longer needed in order to prevent that
+	 * another process can create the same file concurrently using another NFS
+	 * client which might not yet see the file due to caching.
+	 *
+	 * @see "https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html"
+	 * @param file
+	 *            the unique file to be created atomically
+	 * @return LockToken this lock token must be held until the file is no
+	 *         longer needed
+	 * @throws IOException
+	 * @since 5.0
+	 */
+	@Override
+	public LockToken createNewFileAtomic(File file) throws IOException {
+		if (!file.createNewFile()) {
+			return token(false, null);
+		}
+		if (supportsAtomicCreateNewFile() || !supportsUnixNLink) {
+			return token(true, null);
+		}
+		Path link = null;
+		Path path = file.toPath();
+		try {
+			link = Files.createLink(Paths.get(uniqueLinkPath(file)), path);
+			Integer nlink = (Integer) (Files.getAttribute(path,
+					"unix:nlink")); //$NON-NLS-1$
+			if (nlink.intValue() > 2) {
+				LOG.warn(MessageFormat.format(
+						JGitText.get().failedAtomicFileCreation, path, nlink));
+				return token(false, link);
+			} else if (nlink.intValue() < 2) {
+				supportsUnixNLink = false;
+			}
+			return token(true, link);
+		} catch (UnsupportedOperationException | IllegalArgumentException e) {
+			supportsUnixNLink = false;
+			return token(true, link);
+		}
+	}
+
+	private static LockToken token(boolean created, @Nullable Path p) {
+		return ((p != null) && Files.exists(p))
+				? new LockToken(created, Optional.of(p))
+				: new LockToken(created, Optional.empty());
+	}
+
+	private static String uniqueLinkPath(File file) {
+		UUID id = UUID.randomUUID();
+		return file.getAbsolutePath() + "." //$NON-NLS-1$
+				+ Long.toHexString(id.getMostSignificantBits())
+				+ Long.toHexString(id.getLeastSignificantBits());
+	}
 }