fixup! module: implement NODE_COMPILE_CACHE for automatic on-disk code caching

joyeecheung · joyeecheung · commit b45062b1ba1c · 2024-04-17T04:44:31.000+02:00
diff --git a/src/compile_cache.cc b/src/compile_cache.cc
@@ -4,6 +4,7 @@
 #include "node_file.h"
 #include "node_internals.h"
 #include "node_version.h"
+#include "path.h"
 #include "zlib.h"
 
 namespace node {
@@ -27,7 +28,7 @@ uint32_t GetHash(const char* data, size_t size) {
 }
 
 uint32_t GetCacheVersionTag() {
-  std::string node_version(NODE_VERSION);
+  std::string_view node_version(NODE_VERSION);
   uint32_t v8_tag = v8::ScriptCompiler::CachedDataVersionTag();
   uLong crc = crc32(0L, Z_NULL, 0);
   crc = crc32(crc, reinterpret_cast<const Bytef*>(&v8_tag), sizeof(uint32_t));
@@ -119,7 +120,7 @@ void CompileCacheHandler::ReadCacheFile(CompileCacheEntry* entry) {
     return;
   }
 
-  // Read the cache, grow the buffer exponentially whenever it ills up.
+  // Read the cache, grow the buffer exponentially whenever it fills up.
   size_t offset = headers_buf.len;
   size_t capacity = 4096;  // Initial buffer capacity
   size_t total_read = 0;
@@ -340,10 +341,33 @@ CompileCacheHandler::CompileCacheHandler(Environment* env)
 //     - <cache_file_1>: a hash of filename + module type
 //     - <cache_file_2>
 //     - <cache_file_3>
-bool CompileCacheHandler::InitializeDirectory(const std::string& dir) {
+bool CompileCacheHandler::InitializeDirectory(Environment* env,
+                                              const std::string& dir) {
   compiler_cache_key_ = GetCacheVersionTag();
-  std::string cache_dir =
-      dir + kPathSeparator + Uint32ToHex(compiler_cache_key_);
+  std::string compiler_cache_key_string = Uint32ToHex(compiler_cache_key_);
+  std::vector<std::string_view> paths = {dir, compiler_cache_key_string};
+  std::string cache_dir = PathResolve(env, paths);
+
+  Debug("[compile cache] resolved path %s + %s -> %s\n",
+        dir,
+        compiler_cache_key_string,
+        cache_dir);
+
+  if (UNLIKELY(!env->permission()->is_granted(
+          permission::PermissionScope::kFileSystemWrite, cache_dir))) {
+    Debug("[compile cache] skipping cache because write permission for %s "
+          "is not granted\n",
+          cache_dir);
+    return false;
+  }
+
+  if (UNLIKELY(!env->permission()->is_granted(
+          permission::PermissionScope::kFileSystemRead, cache_dir))) {
+    Debug("[compile cache] skipping cache because read permission for %s "
+          "is not granted\n",
+          cache_dir);
+    return false;
+  }
 
   fs::FSReqWrapSync req_wrap;
   int err = fs::MKDirpSync(nullptr, &(req_wrap.req), cache_dir, 0777, nullptr);
@@ -356,22 +380,7 @@ bool CompileCacheHandler::InitializeDirectory(const std::string& dir) {
     return false;
   }
 
-  uv_fs_t req;
-  auto clean = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
-  err = uv_fs_realpath(nullptr, &req, cache_dir.data(), nullptr);
-  if (is_debug_) {
-    Debug("[compile cache] resolving real path %s...%s\n",
-          cache_dir,
-          err < 0 ? uv_strerror(err) : "success");
-  }
-  if (err != 0 && err != UV_ENOENT) {
-    return false;
-  }
-
-  compile_cache_dir_ = std::string(static_cast<char*>(req.ptr));
-  Debug("[compile cache] resolved real path %s -> %s\n",
-        cache_dir,
-        compile_cache_dir_);
+  compile_cache_dir_ = cache_dir;
   return true;
 }
 
diff --git a/src/compile_cache.h b/src/compile_cache.h
@@ -36,7 +36,7 @@ struct CompileCacheEntry {
 class CompileCacheHandler {
  public:
   explicit CompileCacheHandler(Environment* env);
-  bool InitializeDirectory(const std::string& dir);
+  bool InitializeDirectory(Environment* env, const std::string& dir);
 
   void Persist();
 
diff --git a/src/env.cc b/src/env.cc
@@ -1097,7 +1097,7 @@ void Environment::InitializeCompileCache() {
     return;
   }
   auto handler = std::make_unique<CompileCacheHandler>(this);
-  if (handler->InitializeDirectory(dir_from_env)) {
+  if (handler->InitializeDirectory(this, dir_from_env)) {
     compile_cache_handler_ = std::move(handler);
     AtExit(
         [](void* env) {
diff --git a/test/parallel/test-compile-cache-bad-syntax.js b/test/parallel/test-compile-cache-bad-syntax.js
@@ -0,0 +1,54 @@
+'use strict';
+
+// This tests NODE_COMPILE_CACHE works.
+
+require('../common');
+const { spawnSyncAndExit } = require('../common/child_process');
+const fixtures = require('../common/fixtures');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+
+{
+  // Test that it throws if the script fails to parse, and no cache is created.
+  tmpdir.refresh();
+  const dir = tmpdir.resolve('.compile_cache_dir');
+
+  spawnSyncAndExit(
+    process.execPath,
+    [fixtures.path('syntax', 'bad_syntax.js')],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: dir
+      },
+      cwd: tmpdir.path
+    },
+    {
+      status: 1,
+      stderr: /skip .*bad_syntax\.js because the cache was not initialized/,
+    });
+
+  const cacheDir = fs.readdirSync(dir);
+  assert.strictEqual(cacheDir.length, 1);
+  const entries = fs.readdirSync(path.join(dir, cacheDir[0]));
+  assert.strictEqual(entries.length, 0);
+
+  spawnSyncAndExit(
+    process.execPath,
+    [fixtures.path('syntax', 'bad_syntax.mjs')],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: dir
+      },
+      cwd: tmpdir.path
+    },
+    {
+      status: 1,
+      stderr: /skip .*bad_syntax\.mjs because the cache was not initialized/,
+    });
+}
diff --git a/test/parallel/test-compile-cache-permission-allowed.js b/test/parallel/test-compile-cache-permission-allowed.js
@@ -0,0 +1,78 @@
+'use strict';
+
+// This tests NODE_COMPILE_CACHE works in existing directory.
+
+require('../common');
+const { spawnSyncAndAssert } = require('../common/child_process');
+const assert = require('assert');
+const tmpdir = require('../common/tmpdir');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+
+function testAllowed(readDir, writeDir, envDir) {
+  console.log(readDir, writeDir, envDir);  // Logging for debugging.
+
+  tmpdir.refresh();
+  const dummyDir = tmpdir.resolve('dummy');
+  fs.mkdirSync(dummyDir);
+  const script = tmpdir.resolve(dummyDir, 'empty.js');
+  fs.copyFileSync(fixtures.path('empty.js'), script);
+  // If the directory doesn't exist, permission will just be disallowed.
+  fs.mkdirSync(tmpdir.resolve(envDir));
+
+  spawnSyncAndAssert(
+    process.execPath,
+    [
+      '--experimental-permission',
+      `--allow-fs-read=${dummyDir}`,
+      `--allow-fs-read=${readDir}`,
+      `--allow-fs-write=${writeDir}`,
+      script,
+    ],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: `${envDir}`
+      },
+      cwd: tmpdir.path
+    },
+    {
+      stderr(output) {
+        assert.match(output, /writing cache for .*empty\.js.*success/);
+        return true;
+      }
+    });
+
+  spawnSyncAndAssert(
+    process.execPath,
+    [
+      '--experimental-permission',
+      `--allow-fs-read=${dummyDir}`,
+      `--allow-fs-read=${readDir}`,
+      `--allow-fs-write=${writeDir}`,
+      script,
+    ],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: `${envDir}`
+      },
+      cwd: tmpdir.path
+    },
+    {
+      stderr(output) {
+        assert.match(output, /cache for .*empty\.js was accepted/);
+        return true;
+      }
+    });
+}
+
+{
+  testAllowed(tmpdir.resolve('.compile_cache'), tmpdir.resolve('.compile_cache'), '.compile_cache');
+  testAllowed(tmpdir.resolve('.compile_cache'), tmpdir.resolve('.compile_cache'), tmpdir.resolve('.compile_cache'));
+  testAllowed('*', '*', '.compile_cache');
+  testAllowed('*', tmpdir.resolve('.compile_cache'), '.compile_cache');
+  testAllowed(tmpdir.resolve('.compile_cache'), '*', '.compile_cache');
+}
diff --git a/test/parallel/test-compile-cache-permission-disallowed.js b/test/parallel/test-compile-cache-permission-disallowed.js
@@ -0,0 +1,100 @@
+'use strict';
+
+// This tests NODE_COMPILE_CACHE works in existing directory.
+
+require('../common');
+const { spawnSyncAndAssert } = require('../common/child_process');
+const assert = require('assert');
+const tmpdir = require('../common/tmpdir');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+
+function testDisallowed(dummyDir, cacheDirInPermission, cacheDirInEnv) {
+  console.log(dummyDir, cacheDirInPermission, cacheDirInEnv);  // Logging for debugging.
+
+  tmpdir.refresh();
+  const script = tmpdir.resolve(dummyDir, 'empty.js');
+  fs.mkdirSync(tmpdir.resolve(dummyDir));
+  fs.copyFileSync(fixtures.path('empty.js'), script);
+  // If the directory doesn't exist, permission will just be disallowed.
+  if (cacheDirInPermission !== '*') {
+    fs.mkdirSync(tmpdir.resolve(cacheDirInPermission));
+  }
+
+  spawnSyncAndAssert(
+    process.execPath,
+    [
+      '--experimental-permission',
+      `--allow-fs-read=${dummyDir}`,  // No read or write permission for cache dir.
+      `--allow-fs-write=${dummyDir}`,
+      script,
+    ],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: `${cacheDirInEnv}`
+      },
+      cwd: tmpdir.path
+    },
+    {
+      stderr(output) {
+        assert.match(output, /skipping cache because write permission for .* is not granted/);
+        return true;
+      }
+    });
+
+  spawnSyncAndAssert(
+    process.execPath,
+    [
+      '--experimental-permission',
+      `--allow-fs-read=${dummyDir}`,
+      `--allow-fs-read=${cacheDirInPermission}`,  // Read-only
+      `--allow-fs-write=${dummyDir}`,
+      script,
+    ],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: `${cacheDirInEnv}`
+      },
+      cwd: tmpdir.path
+    },
+    {
+      stderr(output) {
+        assert.match(output, /skipping cache because write permission for .* is not granted/);
+        return true;
+      }
+    });
+
+  spawnSyncAndAssert(
+    process.execPath,
+    [
+      '--experimental-permission',
+      `--allow-fs-read=${dummyDir}`,
+      `--allow-fs-write=${cacheDirInPermission}`,  // Write-only
+      script,
+    ],
+    {
+      env: {
+        ...process.env,
+        NODE_DEBUG_NATIVE: 'COMPILE_CACHE',
+        NODE_COMPILE_CACHE: `${cacheDirInEnv}`
+      },
+      cwd: tmpdir.path
+    },
+    {
+      stderr(output) {
+        assert.match(output, /skipping cache because read permission for .* is not granted/);
+        return true;
+      }
+    });
+}
+
+{
+  testDisallowed(tmpdir.resolve('dummy'), tmpdir.resolve('.compile_cache') + '/', '.compile_cache');
+  testDisallowed(tmpdir.resolve('dummy'), tmpdir.resolve('.compile_cache/') + '/', tmpdir.resolve('.compile_cache'));
+  testDisallowed(tmpdir.resolve('dummy'), '*', '.compile_cache');
+  testDisallowed(tmpdir.resolve('dummy'), '*', tmpdir.resolve('.compile_cache'));
+}

Original file line number	Diff line number	Diff line change
`@@ -1097,7 +1097,7 @@ void Environment::InitializeCompileCache() {`
`1097`	`1097`	`return;`
`1098`	`1098`	`}`
`1099`	`1099`	`auto handler = std::make_unique<CompileCacheHandler>(this);`
`1100`		`- if (handler->InitializeDirectory(dir_from_env)) {`
	`1100`	`+ if (handler->InitializeDirectory(this, dir_from_env)) {`
`1101`	`1101`	`compile_cache_handler_ = std::move(handler);`
`1102`	`1102`	`AtExit(`
`1103`	`1103`	`[](void* env) {`