Commit fae6290387850bfddbe4e9c1255750a65568b763 | zig

author	Jacob Young <jacobly@ziglang.org> 2023-05-10 01:52:33-04:00
committer	Jacob Young <jacobly@ziglang.org> 2023-05-11 00:55:40-04:00
log	fae6290387850bfddbe4e9c1255750a65568b763
tree	3812c9ca4ee0436fe3dd77b1d7b237313fb666bb
parent	2d2d79a05b8423c7638348dc5a89793c1e0cafce

Cache: fix race condition

When checking a cache entry with no input files for a hit, if
`createFile` returned `error.WouldBlock` we would forget about the fact
that the file has been created, and all future checks will assume that a
cache hit has happened, even though one never has or does, leading to
rare `FileNotFound` errors trying the access the protected files.

This fix works by writing an extra byte to the manifest file to
distinguish hits and misses when there no input files to write.

1 files changed, 48 insertions(+), 76 deletions(-)

lib
- std
  - Build
    Cache.zig

lib/std/Build/Cache.zig+48-76

 @@ -145,6 +145,8 @@ pub const bin_digest_len = 16;
 pub const hex_digest_len = bin_digest_len * 2;
 pub const BinDigest = [bin_digest_len]u8;
+/// This is currently just an arbitrary non-empty string that can't match another manifest line.
+const manifest_header = "0";
 const manifest_file_size_max = 50 * 1024 * 1024;
 /// The type used for hashing file contents. Currently, this is SipHash128(1, 3), because it
 @@ -152,8 +154,15 @@ const manifest_file_size_max = 50 * 1024 * 1024;
 /// fastest options right now.
 pub const Hasher = crypto.auth.siphash.SipHash128(1, 3);
-/// Initial state, that can be copied.
+/// Initial state with random bytes, that can be copied.
-pub const hasher_init: Hasher = Hasher.init(&[_]u8{0} ** Hasher.key_length);
+/// Refresh this with new random bytes when the manifest
+/// format is modified in a non-backwards-compatible way.
+pub const hasher_init: Hasher = Hasher.init(&[_]u8{
+    0x33, 0x52, 0xa2, 0x84,
+    0xcf, 0x17, 0x56, 0x57,
+    0x01, 0xbb, 0xcd, 0xe4,
+    0x77, 0xd6, 0xf0, 0x60,
+});
 pub const File = struct {
     prefixed_path: ?PrefixedPath,
 @@ -391,72 +400,29 @@ pub const Manifest = struct {
         @memcpy(manifest_file_path[0..self.hex_digest.len], &self.hex_digest);
         manifest_file_path[hex_digest_len..][0..ext.len].* = ext.*;
-        if (self.files.items.len == 0) {
+        while (true) {
-            // If there are no file inputs, we check if the manifest file exists instead of
+            if (self.cache.manifest_dir.createFile(&manifest_file_path, .{
-            // comparing the hashes on the files used for the cached item
+                .read = true,
-            while (true) {
+                .truncate = false,
-                if (self.cache.manifest_dir.openFile(&manifest_file_path, .{
+                .lock = .Exclusive,
-                    .mode = .read_write,
+                .lock_nonblocking = self.want_shared_lock,
-                    .lock = .Exclusive,
+            })) |manifest_file| {
-                    .lock_nonblocking = self.want_shared_lock,
+                self.manifest_file = manifest_file;
-                })) |manifest_file| {
+                self.have_exclusive_lock = true;
-                    self.manifest_file = manifest_file;
+                break;
-                    self.have_exclusive_lock = true;
+            } else |err| switch (err) {
-                    break;
+                error.WouldBlock => {
-                } else |open_err| switch (open_err) {
+                    self.manifest_file = try self.cache.manifest_dir.openFile(&manifest_file_path, .{
-                    error.WouldBlock => {
+                        .mode = .read_write,
-                        self.manifest_file = try self.cache.manifest_dir.openFile(&manifest_file_path, .{
+                        .lock = .Shared,
-                            .lock = .Shared,
+                    });
-                        });
-                        break;
-                    },
-                    error.FileNotFound => {
-                        if (self.cache.manifest_dir.createFile(&manifest_file_path, .{
-                            .read = true,
-                            .truncate = false,
-                            .lock = .Exclusive,
-                            .lock_nonblocking = self.want_shared_lock,
-                        })) |manifest_file| {
-                            self.manifest_file = manifest_file;
-                            self.manifest_dirty = true;
-                            self.have_exclusive_lock = true;
-                            return false; // cache miss; exclusive lock already held
-                        } else |err| switch (err) {
-                            // There are no dir components, so you would think
-                            // that this was unreachable, however we have
-                            // observed on macOS two processes racing to do
-                            // openat() with O_CREAT manifest in ENOENT.
-                            error.WouldBlock, error.FileNotFound => continue,
-                            else => |e| return e,
-                        }
-                    },
-                    else => |e| return e,
-                }
-            }
-        } else {
-            while (true) {
-                if (self.cache.manifest_dir.createFile(&manifest_file_path, .{
-                    .read = true,
-                    .truncate = false,
-                    .lock = .Exclusive,
-                    .lock_nonblocking = self.want_shared_lock,
-                })) |manifest_file| {
-                    self.manifest_file = manifest_file;
-                    self.have_exclusive_lock = true;
                     break;
-                } else |err| switch (err) {
+                },
-                    error.WouldBlock => {
+                // There are no dir components, so you would think that this was
-                        self.manifest_file = try self.cache.manifest_dir.openFile(&manifest_file_path, .{
+                // unreachable, however we have observed on macOS two processes racing
-                            .lock = .Shared,
+                // to do openat() with O_CREAT manifest in ENOENT.
-                        });
+                error.FileNotFound => continue,
-                        break;
+                else => |e| return e,
-                    },
-                    // There are no dir components, so you would think that this was
-                    // unreachable, however we have observed on macOS two processes racing
-                    // to do openat() with O_CREAT manifest in ENOENT.
-                    error.FileNotFound => continue,
-                    else => |e| return e,
-                }
             }
         }
 @@ -469,6 +435,18 @@ pub const Manifest = struct {
         var any_file_changed = false;
         var line_iter = mem.tokenize(u8, file_contents, "\n");
         var idx: usize = 0;
+        if (if (line_iter.next()) |line| !std.mem.eql(u8, line, manifest_header) else true) {
+            self.manifest_dirty = true;
+            while (idx < input_file_count) : (idx += 1) {
+                const ch_file = &self.files.items[idx];
+                self.populateFileHash(ch_file) catch |err| {
+                    self.failed_file_index = idx;
+                    return err;
+                };
+            }
+            try self.upgradeToExclusiveLock();
+            return false;
+        }
         while (line_iter.next()) |line| {
             defer idx += 1;
 @@ -854,19 +832,13 @@ pub const Manifest = struct {
             defer contents.deinit();
             const writer = contents.writer();
-            var encoded_digest: [hex_digest_len]u8 = undefined;
+            try writer.writeAll(manifest_header ++ "\n");
             for (self.files.items) |file| {
-                _ = fmt.bufPrint(
+                try writer.print("{d} {d} {d} {} {d} {s}\n", .{
-                    &encoded_digest,
-                    "{s}",
-                    .{fmt.fmtSliceHexLower(&file.bin_digest)},
-                ) catch unreachable;
-                try writer.print("{d} {d} {d} {s} {d} {s}\n", .{
                     file.stat.size,
                     file.stat.inode,
                     file.stat.mtime,
-                    &encoded_digest,
+                    fmt.fmtSliceHexLower(&file.bin_digest),
                     file.prefixed_path.?.prefix,
                     file.prefixed_path.?.sub_path,
                 });