Fix NFS object corruption

Several months ago I reported a problem with NFS corruption from three
simultaneous NFS users of ccache on the same file; two writers to the cache
and one reader.

I believe I have tracked this issue down to a race related to the use of
unlink. On NFS, unlink() is NOT atomic; so what seemed to be happening was
the second writer unlink()ed the first's object, then the reader got the
partially unlinked (truncated) object.

The following patch fixes this issue by always calling rename before a
unlink - a new x_unlink function. There are some places where temp files
are being unlinked; for performance these can remain as the ordinary
unlink.

diff --git a/HACKING.txt b/HACKING.txt
index 43ff89e9c9c1d3c7fd878168b5ddf7da480a7b9c..b34c29e6ce158f0a3defd2a2c5beff4e0bee92b5 100644 (file)
--- a/HACKING.txt
+++ b/HACKING.txt
@@ -27,6 +27,8 @@ Idioms
 * Use str_eq() instead of strcmp() when testing for string (in)equality.
 * Consider using str_startswith() instead of strncmp().
 * Use bool, true and false for boolean values.
+* Use tmp_unlink() or x_unlink() instead of unlink().
+* Use x_rename() instead of rename().
 
 Other
 -----

diff --git a/ccache.c b/ccache.c
index e4b6239e32738b38aedbf9745180970a395bc24c..efe0cdb04efc4579940c0cea32953afdab27caf9 100644 (file)
--- a/ccache.c
+++ b/ccache.c
@@ -233,7 +233,7 @@ clean_up_tmp_files()
        /* delete intermediate pre-processor file if needed */
        if (i_tmpfile) {
                if (!direct_i_file) {
-                       unlink(i_tmpfile);
+                       tmp_unlink(i_tmpfile);
                }
                free(i_tmpfile);
                i_tmpfile = NULL;
@@ -241,7 +241,7 @@ clean_up_tmp_files()
 
        /* delete the cpp stderr file if necessary */
        if (cpp_stderr) {
-               unlink(cpp_stderr);
+               tmp_unlink(cpp_stderr);
                free(cpp_stderr);
                cpp_stderr = NULL;
        }
@@ -537,12 +537,12 @@ to_cache(struct args *args)
        if (stat(tmp_stdout, &st) != 0 || st.st_size != 0) {
                cc_log("Compiler produced stdout");
                stats_update(STATS_STDOUT);
-               unlink(tmp_stdout);
-               unlink(tmp_stderr);
-               unlink(tmp_obj);
+               tmp_unlink(tmp_stdout);
+               tmp_unlink(tmp_stderr);
+               tmp_unlink(tmp_obj);
                failed();
        }
-       unlink(tmp_stdout);
+       tmp_unlink(tmp_stdout);
 
        /*
         * Merge stderr from the preprocessor (if any) and stderr from the real
@@ -579,7 +579,7 @@ to_cache(struct args *args)
                close(fd_cpp_stderr);
                close(fd_real_stderr);
                close(fd_result);
-               unlink(tmp_stderr2);
+               tmp_unlink(tmp_stderr2);
                free(tmp_stderr2);
        }
 
@@ -597,13 +597,13 @@ to_cache(struct args *args)
                                /* we can use a quick method of getting the failed output */
                                copy_fd(fd, 2);
                                close(fd);
-                               unlink(tmp_stderr);
+                               tmp_unlink(tmp_stderr);
                                exit(status);
                        }
                }
 
-               unlink(tmp_stderr);
-               unlink(tmp_obj);
+               tmp_unlink(tmp_stderr);
+               tmp_unlink(tmp_obj);
                failed();
        }
 
@@ -637,7 +637,7 @@ to_cache(struct args *args)
                added_bytes += file_size(&st);
                added_files += 1;
        } else {
-               unlink(tmp_stderr);
+               tmp_unlink(tmp_stderr);
        }
        if (move_uncompressed_file(tmp_obj, cached_obj, enable_compression) != 0) {
                cc_log("Failed to move %s to %s", tmp_obj, cached_obj);
@@ -741,9 +741,9 @@ get_object_name_from_cpp(struct args *args, struct mdfour *hash)
 
        if (status != 0) {
                if (!direct_i_file) {
-                       unlink(path_stdout);
+                       tmp_unlink(path_stdout);
                }
-               unlink(path_stderr);
+               tmp_unlink(path_stderr);
                cc_log("Preprocessor gave exit status %d", status);
                stats_update(STATS_PREPROCESSOR);
                failed();
@@ -760,7 +760,7 @@ get_object_name_from_cpp(struct args *args, struct mdfour *hash)
                hash_delimiter(hash, "unifycpp");
                if (unify_hash(hash, path_stdout) != 0) {
                        stats_update(STATS_ERROR);
-                       unlink(path_stderr);
+                       tmp_unlink(path_stderr);
                        cc_log("Failed to unify %s", path_stdout);
                        failed();
                }
@@ -768,7 +768,7 @@ get_object_name_from_cpp(struct args *args, struct mdfour *hash)
                hash_delimiter(hash, "cpp");
                if (!process_preprocessed_file(hash, path_stdout)) {
                        stats_update(STATS_ERROR);
-                       unlink(path_stderr);
+                       tmp_unlink(path_stderr);
                        failed();
                }
        }
@@ -788,7 +788,7 @@ get_object_name_from_cpp(struct args *args, struct mdfour *hash)
                 */
                cpp_stderr = path_stderr;
        } else {
-               unlink(path_stderr);
+               tmp_unlink(path_stderr);
                free(path_stderr);
        }
 
@@ -1031,7 +1031,7 @@ from_cache(enum fromcache_call_mode mode, bool put_object_in_manifest)
        if (str_eq(output_obj, "/dev/null")) {
                ret = 0;
        } else {
-               unlink(output_obj);
+               x_unlink(output_obj);
                /* only make a hardlink if the cache file is uncompressed */
                if (getenv("CCACHE_HARDLINK") && !file_is_compressed(cached_obj)) {
                        ret = link(cached_obj, output_obj);
@@ -1051,17 +1051,17 @@ from_cache(enum fromcache_call_mode mode, bool put_object_in_manifest)
                        stats_update(STATS_ERROR);
                        failed();
                }
-               unlink(output_obj);
-               unlink(cached_stderr);
-               unlink(cached_obj);
-               unlink(cached_dep);
+               x_unlink(output_obj);
+               x_unlink(cached_stderr);
+               x_unlink(cached_obj);
+               x_unlink(cached_dep);
                return;
        } else {
                cc_log("Created %s from %s", output_obj, cached_obj);
        }
 
        if (produce_dep_file) {
-               unlink(output_dep);
+               x_unlink(output_dep);
                /* only make a hardlink if the cache file is uncompressed */
                if (getenv("CCACHE_HARDLINK") && !file_is_compressed(cached_dep)) {
                        ret = link(cached_dep, output_dep);
@@ -1083,11 +1083,11 @@ from_cache(enum fromcache_call_mode mode, bool put_object_in_manifest)
                                stats_update(STATS_ERROR);
                                failed();
                        }
-                       unlink(output_obj);
-                       unlink(output_dep);
-                       unlink(cached_stderr);
-                       unlink(cached_obj);
-                       unlink(cached_dep);
+                       x_unlink(output_obj);
+                       x_unlink(output_dep);
+                       x_unlink(cached_stderr);
+                       x_unlink(cached_obj);
+                       x_unlink(cached_dep);
                        return;
                } else {
                        cc_log("Created %s from %s", output_dep, cached_dep);
@@ -1912,7 +1912,7 @@ ccache(int argc, char *argv[])
                cc_log("Hash from manifest doesn't match preprocessor output");
                cc_log("Likely reason: different CCACHE_BASEDIRs used");
                cc_log("Removing manifest as a safety measure");
-               unlink(manifest_path);
+               x_unlink(manifest_path);
 
                put_object_in_manifest = true;
        }

diff --git a/ccache.h b/ccache.h
index c306ddfc7923551206e3debf831e13720b518e3f..570d74c05dd83ac0a69fc0662138fb996a645a49 100644 (file)
--- a/ccache.h
+++ b/ccache.h
@@ -140,6 +140,8 @@ bool is_absolute_path(const char *path);
 bool is_full_path(const char *path);
 void update_mtime(const char *path);
 int x_rename(const char *oldpath, const char *newpath);
+int x_unlink(const char *path);
+int tmp_unlink(const char *path);
 char *x_readlink(const char *path);
 char *read_text_file(const char *path, size_t size_hint);
 bool read_file(const char *path, size_t size_hint, char **data, size_t *size);

diff --git a/cleanup.c b/cleanup.c
index 1cf729d74371a847e475c932b83037fed1b97e18..530bd45bde107df9b1d12fbc9df24a12ccde950b 100644 (file)
--- a/cleanup.c
+++ b/cleanup.c
@@ -73,7 +73,7 @@ traverse_fn(const char *fname, struct stat *st)
        if (strstr(p, ".tmp.") != NULL) {
                /* delete any tmp files older than 1 hour */
                if (st->st_mtime + 3600 < time(NULL)) {
-                       unlink(fname);
+                       x_unlink(fname);
                        goto out;
                }
        }
@@ -98,7 +98,7 @@ out:
 static void
 delete_file(const char *path, size_t size)
 {
-       if (unlink(path) == 0) {
+       if (x_unlink(path) == 0) {
                cache_size -= size;
                files_in_cache--;
        } else if (errno != ENOENT) {
@@ -242,7 +242,7 @@ static void wipe_fn(const char *fname, struct stat *st)
        }
        free(p);
 
-       unlink(fname);
+       x_unlink(fname);
 }
 
 /* wipe all cached files in all subdirs */

diff --git a/execute.c b/execute.c
index 48addf67789f83163064c8e61a09bffc0769dacf..72e6d4ffdbfc773b1efd1cd4369000ca9af5f69a 100644 (file)
--- a/execute.c
+++ b/execute.c
@@ -185,7 +185,7 @@ execute(char **argv, const char *path_stdout, const char *path_stderr)
        if (pid == 0) {
                int fd;
 
-               unlink(path_stdout);
+               tmp_unlink(path_stdout);
                fd = open(path_stdout, O_WRONLY|O_CREAT|O_TRUNC|O_EXCL|O_BINARY, 0666);
                if (fd == -1) {
                        exit(1);
@@ -193,7 +193,7 @@ execute(char **argv, const char *path_stdout, const char *path_stderr)
                dup2(fd, 1);
                close(fd);
 
-               unlink(path_stderr);
+               tmp_unlink(path_stderr);
                fd = open(path_stderr, O_WRONLY|O_CREAT|O_TRUNC|O_EXCL|O_BINARY, 0666);
                if (fd == -1) {
                        exit(1);

diff --git a/lockfile.c b/lockfile.c
index 14b0cbb8daf04845c6915dd0afdd97fba14b507c..404e5c1c64f24c65f3951679ff86f5f557de031d 100644 (file)
--- a/lockfile.c
+++ b/lockfile.c
@@ -91,7 +91,7 @@ lockfile_acquire(const char *path, unsigned staleness_limit)
                        if (write(fd, my_content, strlen(my_content)) == -1) {
                                cc_log("lockfile_acquire: write %s: %s", lockfile, strerror(errno));
                                close(fd);
-                               unlink(lockfile);
+                               x_unlink(lockfile);
                                goto out;
                        }
                        close(fd);
@@ -192,7 +192,7 @@ lockfile_release(const char *path)
 {
        char *lockfile = format("%s.lock", path);
        cc_log("Releasing lock %s", lockfile);
-       unlink(lockfile);
+       tmp_unlink(lockfile);
        free(lockfile);
 }
 

diff --git a/stats.c b/stats.c
index c98b25b9a0e0d1c407cbfbc15eab14d501f65acd..a445dc524f9e4e76515663b41b9afb359d1b369e 100644 (file)
--- a/stats.c
+++ b/stats.c
@@ -339,7 +339,7 @@ stats_zero(void)
        char *fname;
 
        fname = format("%s/stats", cache_dir);
-       unlink(fname);
+       x_unlink(fname);
        free(fname);
 
        for (dir = 0; dir <= 0xF; dir++) {

diff --git a/util.c b/util.c
index ce36f07c40700026a6460eba109b1c9b0f652b57..5e6979ec111b09320413a34772e5c2222b3ad5d1 100644 (file)
--- a/util.c
+++ b/util.c
@@ -290,7 +290,7 @@ copy_file(const char *src, const char *dest, int compress_dest)
                        gzclose(gz_out);
                }
                close(fd_out);
-               unlink(tmp_name);
+               tmp_unlink(tmp_name);
                free(tmp_name);
                return -1;
        }
@@ -334,7 +334,7 @@ error:
        if (fd_out != -1) {
                close(fd_out);
        }
-       unlink(tmp_name);
+       tmp_unlink(tmp_name);
        free(tmp_name);
        return -1;
 }
@@ -347,7 +347,7 @@ move_file(const char *src, const char *dest, int compress_dest)
 
        ret = copy_file(src, dest, compress_dest);
        if (ret != -1) {
-               unlink(src);
+               x_unlink(src);
        }
        return ret;
 }
@@ -1109,11 +1109,43 @@ x_rename(const char *oldpath, const char *newpath)
 {
 #ifdef _WIN32
        /* Windows' rename() refuses to overwrite an existing file. */
-       unlink(newpath);
+       unlink(newpath);  /* not x_unlink, as x_unlink calls x_rename */
 #endif
        return rename(oldpath, newpath);
 }
 
+/*
+ * Remove path, NFS hazardous, for temporary files that will not exist
+ * on other systems only.  IE the "path" should include tmp_string().
+ */
+int
+tmp_unlink(const char *path)
+{
+       cc_log("Unlink %s (as-tmp)", path);
+       return (unlink(path));
+}
+
+/*
+ * Remove path, safely for NFS
+ */
+int
+x_unlink(const char *path)
+{
+       /* If path is on an NFS share, unlink isn't atomic, so we rename to a temp file */
+       /* We don't care if tempfile is trashed, so it's always safe to unlink it first */
+       const char* tmp_name = format("%s.%s.rmXXXXXX", path, tmp_string());
+       cc_log("Unlink %s via %s", path, tmp_name);
+       if (x_rename(path, tmp_name) == -1) {
+               /* let caller report the error, or not */
+               return -1;
+       }
+       if (unlink(tmp_name) == -1) {
+               /* let caller report the error, or not */
+               return -1;
+       }
+       return 0;
+}
+
 #ifndef _WIN32
 /* Like readlink() but returns the string or NULL on failure. Caller frees. */
 char *