From 06e06fc291a8ccc0e68b7d84b54097a9635df740 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sun, 26 Aug 2018 19:44:29 +0200 Subject: [PATCH] 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 --- .../eclipse/jgit/internal/JGitText.properties | 3 + .../org/eclipse/jgit/internal/JGitText.java | 3 + .../jgit/internal/storage/file/LockFile.java | 29 ++++++- .../src/org/eclipse/jgit/util/FS.java | 71 ++++++++++++++++- .../src/org/eclipse/jgit/util/FS_POSIX.java | 76 ++++++++++++++++++- 5 files changed, 176 insertions(+), 6 deletions(-) diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 16f184babc..873cf526bb 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/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 diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 4b7459c15b..9ea0aa5702 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/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; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java index 584c4714d8..15c5280517 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java +++ b/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(); } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java index 98e73216b3..6b537a4775 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java +++ b/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 true if the file was created, false 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 link; + + LockToken(boolean isCreated, Optional 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$ + : "]"); //$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 false + * + * @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)}. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java index 9f273d1b84..607e078604 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java +++ b/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} + *

+ * 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()); + } } -- 2.39.5