From fdbb329d1009b4c7b23552e65c0344e3ce52b459 Mon Sep 17 00:00:00 2001
From: Andrew Kelley <andrew@ziglang.org>
Date: Thu, 9 Dec 2021 21:14:39 -0700
Subject: Cache: fix data race with is_problematic_timestamp

Previously `recent_problematic_timestamp` was unprotected and accessed
potentially with multiple worker threads simultaneously.

This commit protects it with atomics and also introduces a flag to
prevent multiple timestamp checks from within the same call to hit().

Unfortunately the compiler-rt function __sync_val_compare_and_swap_16 is
not yet implemented, so I will have to take a different strategy in a
follow-up commit.
---
 src/Cache.zig | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

(limited to 'src/Cache.zig')

diff --git a/src/Cache.zig b/src/Cache.zig
index 857ddc78e3..03b44fc535 100644
--- a/src/Cache.zig
+++ b/src/Cache.zig
@@ -2,6 +2,7 @@ gpa: Allocator,
 manifest_dir: fs.Dir,
 hash: HashHelper = .{},
 recent_problematic_timestamp: i128 = 0,
+want_refresh_timestamp: bool = true,
 
 const Cache = @This();
 const std = @import("std");
@@ -346,6 +347,12 @@ pub const Manifest = struct {
             }
         }
 
+        // Indicate that we want isProblematicTimestamp to perform a filesystem write in
+        // order to obtain a problematic timestamp for the next call. Calls after that
+        // in this same hit() function call will then use the same timestamp, to avoid
+        // writing multiple times to the filesystem.
+        @atomicStore(bool, &self.cache.want_refresh_timestamp, true, .Monotonic);
+
         const file_contents = try self.manifest_file.?.reader().readAllAlloc(self.cache.gpa, manifest_file_size_max);
         defer self.cache.gpa.free(file_contents);
 
@@ -749,18 +756,29 @@ fn hashFile(file: fs.File, bin_digest: *[Hasher.mac_length]u8) !void {
 fn isProblematicTimestamp(cache: *Cache, file_time: i128) bool {
     // If the file_time is prior to the most recent problematic timestamp
     // then we don't need to access the filesystem.
-    if (file_time < cache.recent_problematic_timestamp)
+    var ts = @atomicLoad(i128, &cache.recent_problematic_timestamp, .Monotonic);
+    if (file_time < ts)
         return false;
 
-    var file = cache.manifest_dir.createFile("timestamp", .{
-        .read = true,
-        .truncate = true,
-    }) catch return true;
-    defer file.close();
+    if (@atomicRmw(bool, &cache.want_refresh_timestamp, .Xchg, false, .Monotonic)) {
+        var file = cache.manifest_dir.createFile("timestamp", .{
+            .read = true,
+            .truncate = true,
+        }) catch return true;
+        defer file.close();
 
-    cache.recent_problematic_timestamp = (file.stat() catch return true).mtime;
+        const new_ts = (file.stat() catch return true).mtime;
+        ts = if (@cmpxchgWeak(
+            i128,
+            &cache.recent_problematic_timestamp,
+            ts,
+            new_ts,
+            .Monotonic,
+            .Monotonic,
+        )) |race_ts| race_ts else new_ts;
+    }
 
-    return file_time >= cache.recent_problematic_timestamp;
+    return file_time >= ts;
 }
 
 // Create/Write a file, close it, then grab its stat.mtime timestamp.
-- 
cgit v1.2.3