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 e0fbe7685b7ff81b6658d5a142a1c37cf5127380..0597790b6ae331a4badfd567678c166ff1bc3713 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 e9601b6fb22d199cfa0a05fe58c20d5fa45edb8c..05f8631cdfceeecf0c9a231ecaf3c97a95e8fa1a 100644 (file)
--- a/ccache.c
+++ b/ccache.c
@@ -236,7 +236,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;
@@ -244,7 +244,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;
        }
@@ -519,12 +519,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
@@ -561,7 +561,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);
        }
 
@@ -579,13 +579,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();
        }
 
@@ -619,7 +619,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);
@@ -703,9 +703,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();
@@ -722,7 +722,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();
                }
@@ -730,7 +730,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();
                }
        }
@@ -750,7 +750,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);
        }
 
@@ -993,7 +993,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);
@@ -1013,17 +1013,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);
@@ -1045,11 +1045,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);
@@ -1874,7 +1874,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 9643b6c562d2dfca997d0e5264c6647b4ace6071..68ecdb8524c4b83f23d2f70bb9a1fb607ae1d831 100644 (file)
--- a/ccache.h
+++ b/ccache.h
@@ -139,6 +139,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);
 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 a97239195ae7cc39026a6d05fcf8c991ac2ee7d2..d68ad58f411bc52c437092638c411004ae10ec63 100644 (file)
--- a/execute.c
+++ b/execute.c
@@ -175,7 +175,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);
@@ -183,7 +183,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 e62aac4be3d52d5e5a133b47e4dbf070070cc2c5..ef48e19a033ad07bdca9447015f9e9200e44611e 100644 (file)
--- a/lockfile.c
+++ b/lockfile.c
@@ -84,7 +84,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);
@@ -178,7 +178,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 1494007f612fccc0be964947328f8568a31c86c6..e458cb103ddd3b6df110b0f943df3fad90801ac9 100644 (file)
--- a/stats.c
+++ b/stats.c
@@ -335,7 +335,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 39cf7ccd3f02c092605fedfe6c4a242779f8ee4d..5091a8fd3ef0b769f2b18471956eb33cf2dd6610 100644 (file)
--- a/util.c
+++ b/util.c
@@ -268,7 +268,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;
        }
@@ -312,7 +312,7 @@ error:
        if (fd_out != -1) {
                close(fd_out);
        }
-       unlink(tmp_name);
+       tmp_unlink(tmp_name);
        free(tmp_name);
        return -1;
 }
@@ -325,7 +325,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;
 }
@@ -1048,11 +1048,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 *