From b6c989b80198badf5f694340c07abc282365aaec Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 5 Aug 2012 21:41:44 -0400 Subject: [PATCH] Fix race condition when two processes create a hard link to a file in .links This is a problem because one process may set the immutable bit before the second process has created its link. Addressed random Hydra failures such as: error: cannot rename `/nix/store/.tmp-link-17397-1804289383' to `/nix/store/rsvzm574rlfip3830ac7kmaa028bzl6h-nixos-0.1pre-git/upstart-interface-version': Operation not permitted --- src/libstore/optimise-store.cc | 64 ++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index b9b878d2a..35c8dd48f 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -123,9 +123,6 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) printMsg(lvlTalkative, format("linking `%1%' to `%2%'") % path % linkPath); - Path tempLink = (format("%1%/.tmp-link-%2%-%3%") - % nixStore % getpid() % rand()).str(); - /* Make the containing directory writable, but only if it's not the store itself (we don't want or need to mess with its permissions). */ @@ -140,40 +137,53 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) so make it mutable first (and make it immutable again when we're done). We also have to make ‘path’ mutable, otherwise rename() will fail to delete it. */ - makeMutable(linkPath); - MakeImmutable mk1(linkPath); - makeMutable(path); MakeImmutable mk2(path); - if (link(linkPath.c_str(), tempLink.c_str()) == -1) { - if (errno == EMLINK) { - /* Too many links to the same file (>= 32000 on most file - systems). This is likely to happen with empty files. - Just shrug and ignore. */ - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - return; + /* Another process might be doing the same thing (creating a new + link to ‘linkPath’) and make ‘linkPath’ immutable before we're + done. In that case, just retry. */ + unsigned int retries = 1024; + while (--retries > 0) { + makeMutable(linkPath); + MakeImmutable mk1(linkPath); + + Path tempLink = (format("%1%/.tmp-link-%2%-%3%") + % nixStore % getpid() % rand()).str(); + + if (link(linkPath.c_str(), tempLink.c_str()) == -1) { + if (errno == EMLINK) { + /* Too many links to the same file (>= 32000 on most + file systems). This is likely to happen with empty + files. Just shrug and ignore. */ + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; + } + if (errno == EPERM) continue; + throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); } - throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); - } - /* Atomically replace the old file with the new hard link. */ - if (rename(tempLink.c_str(), path.c_str()) == -1) { - if (errno == EMLINK) { - /* Some filesystems generate too many links on the rename, - rather than on the original link. (Probably it - temporarily increases the st_nlink field before - decreasing it again.) */ - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - - /* Unlink the temp link. */ + /* Atomically replace the old file with the new hard link. */ + if (rename(tempLink.c_str(), path.c_str()) == -1) { if (unlink(tempLink.c_str()) == -1) printMsg(lvlError, format("unable to unlink `%1%'") % tempLink); - return; + if (errno == EMLINK) { + /* Some filesystems generate too many links on the + rename, rather than on the original link. + (Probably it temporarily increases the st_nlink + field before decreasing it again.) */ + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; + } + if (errno == EPERM) continue; + throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); } - throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); + + break; } + if (retries == 0) throw Error(format("cannot link `%1%' to `%2%'") % path % linkPath); + stats.filesLinked++; stats.bytesFreed += st.st_size; stats.blocksFreed += st.st_blocks;