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.  I
found it easier to never use unlink() and instead either use
the safe x_unlink() or the dangerous tmp_unlink(), but
understand if you want to only take the x_unlink part of the
patch.

Thanks much.

-Wilson

diff --git a/HACKING.txt b/HACKING.txt
index 43ff89e..b34c29e 100644
--- 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 e4b6239..efe0cdb 100644
--- 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 c306ddf..570d74c 100644
--- 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 1cf729d..530bd45 100644
--- 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 48addf6..72e6d4f 100644
--- 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 14b0cbb..404e5c1 100644
--- 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 c98b25b..a445dc5 100644
--- 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 ce36f07..37d1688 100644
--- a/util.c
+++ b/util.c
@@ -290,7 +292,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 +348,7 @@ error:
        if (fd_out != -1) {
                close(fd_out);
        }
-       unlink(tmp_name);
+       tmp_unlink(tmp_name);
        free(tmp_name);
        return -1;
 }
@@ -347,7 +361,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 +1123,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 *
_______________________________________________
ccache mailing list
ccache@lists.samba.org
https://lists.samba.org/mailman/listinfo/ccache

Reply via email to