From 14a1de04239606d33221a99e2e0199182bb74f67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= Date: Mon, 1 Jul 2024 16:36:51 +0300 Subject: [PATCH 1/3] module,win: fix long path resolve --- src/node_file.cc | 20 +- src/node_modules.cc | 46 +++-- test/es-module/test-GH-50753.js | 42 ---- test/es-module/test-cjs-esm-warn.js | 2 +- .../test-cjs-legacyMainResolve-permission.js | 18 +- test/es-module/test-esm-long-path-win.js | 189 ++++++++++++++++++ 6 files changed, 251 insertions(+), 66 deletions(-) delete mode 100644 test/es-module/test-GH-50753.js create mode 100644 test/es-module/test-esm-long-path-win.js diff --git a/src/node_file.cc b/src/node_file.cc index 41ac3d37cd1b0c..30e1fcbdde8ded 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3096,8 +3096,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { for (int i = 0; i < legacy_main_extensions_with_main_end; i++) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); - - switch (FilePathIsFile(env, file_path)) { + Local local_file_path = + Buffer::Copy( + env->isolate(), file_path.c_str(), strlen(file_path.c_str())) + .ToLocalChecked(); + BufferValue buff_file_path(isolate, local_file_path); + ToNamespacedPath(env, &buff_file_path); + + switch (FilePathIsFile(env, buff_file_path.ToString())) { case BindingData::FilePathIsFileReturnType::kIsFile: return args.GetReturnValue().Set(i); case BindingData::FilePathIsFileReturnType::kIsNotFile: @@ -3133,8 +3139,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { i < legacy_main_extensions_package_fallback_end; i++) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); - - switch (FilePathIsFile(env, file_path)) { + Local local_file_path = + Buffer::Copy( + env->isolate(), file_path.c_str(), strlen(file_path.c_str())) + .ToLocalChecked(); + BufferValue buff_file_path(isolate, local_file_path); + ToNamespacedPath(env, &buff_file_path); + + switch (FilePathIsFile(env, buff_file_path.ToString())) { case BindingData::FilePathIsFileReturnType::kIsFile: return args.GetReturnValue().Set(i); case BindingData::FilePathIsFileReturnType::kIsNotFile: diff --git a/src/node_modules.cc b/src/node_modules.cc index 2e80ff4e95a5c1..88252ccf7b8a2d 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -4,6 +4,7 @@ #include "node_errors.h" #include "node_external_reference.h" #include "node_url.h" +#include "path.h" #include "permission/permission.h" #include "permission/permission_base.h" #include "util-inl.h" @@ -241,7 +242,7 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); auto isolate = realm->isolate(); - Utf8Value path(isolate, args[0]); + BufferValue path(isolate, args[0]); bool is_esm = args[1]->IsTrue(); auto error_context = ErrorContext(); if (is_esm) { @@ -261,15 +262,10 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo& args) { permission::PermissionScope::kFileSystemRead, path.ToStringView()); - // TODO(StefanStojanovic): Remove ifdef after - // path.toNamespacedPath logic is ported to C++ -#ifdef _WIN32 - auto package_json = GetPackageJSON( - realm, "\\\\?\\" + path.ToString(), is_esm ? &error_context : nullptr); -#else + ToNamespacedPath(realm->env(), &path); auto package_json = GetPackageJSON(realm, path.ToString(), is_esm ? &error_context : nullptr); -#endif + if (package_json == nullptr) { return; } @@ -323,9 +319,22 @@ void BindingData::GetNearestParentPackageJSON( CHECK(args[0]->IsString()); Realm* realm = Realm::GetCurrent(args); - Utf8Value path_value(realm->isolate(), args[0]); + BufferValue path_value(realm->isolate(), args[0]); + // Check if the path has a trailing slash. If so, add it after + // ToNamespacedPath() as it will be deleted by ToNamespacedPath() + bool slashCheck = !path_value.ToString().empty() && + path_value.ToString().back() == + std::filesystem::path::preferred_separator; + + ToNamespacedPath(realm->env(), &path_value); + + std::string path_value_str = path_value.ToString(); + if (slashCheck) { + path_value_str.push_back(std::filesystem::path::preferred_separator); + } + auto package_json = - TraverseParent(realm, std::filesystem::path(path_value.ToString())); + TraverseParent(realm, std::filesystem::path(path_value_str)); if (package_json != nullptr) { args.GetReturnValue().Set(package_json->Serialize(realm)); @@ -338,9 +347,22 @@ void BindingData::GetNearestParentPackageJSONType( CHECK(args[0]->IsString()); Realm* realm = Realm::GetCurrent(args); - Utf8Value path(realm->isolate(), args[0]); + BufferValue path_value(realm->isolate(), args[0]); + // Check if the path has a trailing slash. If so, add it after + // ToNamespacedPath() as it will be deleted by ToNamespacedPath() + bool slashCheck = !path_value.ToString().empty() && + path_value.ToString().back() == + std::filesystem::path::preferred_separator; + + ToNamespacedPath(realm->env(), &path_value); + + std::string path_value_str = path_value.ToString(); + if (slashCheck) { + path_value_str.push_back(std::filesystem::path::preferred_separator); + } + auto package_json = - TraverseParent(realm, std::filesystem::path(path.ToString())); + TraverseParent(realm, std::filesystem::path(path_value_str)); if (package_json == nullptr) { return; diff --git a/test/es-module/test-GH-50753.js b/test/es-module/test-GH-50753.js deleted file mode 100644 index 1cecfa5bebf6a5..00000000000000 --- a/test/es-module/test-GH-50753.js +++ /dev/null @@ -1,42 +0,0 @@ -'use strict'; - -// Flags: --expose-internals - -const common = require('../common'); -if (!common.isWindows) { - common.skip('this test is Windows-specific.'); -} - -const fs = require('fs'); -const path = require('path'); -const tmpdir = require('../common/tmpdir'); - -// https://github.com/nodejs/node/issues/50753 -// Make a path that is more than 260 chars long. -// Module layout will be the following: -// package.json -// main -// main.js - -const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); -const packageDirName = tmpdir.resolve('x'.repeat(packageDirNameLen)); -const packageDirPath = path.resolve(packageDirName); -const packageJsonFilePath = path.join(packageDirPath, 'package.json'); -const mainDirName = 'main'; -const mainDirPath = path.resolve(packageDirPath, mainDirName); -const mainJsFile = 'main.js'; -const mainJsFilePath = path.resolve(mainDirPath, mainJsFile); - -tmpdir.refresh(); - -fs.mkdirSync(packageDirPath); -fs.writeFileSync( - packageJsonFilePath, - `{"main":"${mainDirName}/${mainJsFile}"}` -); -fs.mkdirSync(mainDirPath); -fs.writeFileSync(mainJsFilePath, 'console.log("hello world");'); - -require('internal/modules/run_main').executeUserEntryPoint(packageDirPath); - -tmpdir.refresh(); diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index 29d83ef92ce8d0..ccd4d276fdd535 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -10,7 +10,7 @@ const { describe, it } = require('node:test'); const requiringCjsAsEsm = path.resolve(fixtures.path('/es-modules/cjs-esm.js')); const requiringEsm = path.resolve(fixtures.path('/es-modules/cjs-esm-esm.js')); -const pjson = path.resolve( +const pjson = path.toNamespacedPath( fixtures.path('/es-modules/package-type-module/package.json') ); diff --git a/test/es-module/test-cjs-legacyMainResolve-permission.js b/test/es-module/test-cjs-legacyMainResolve-permission.js index b45e3dee3bbfcd..392bfb753d7764 100644 --- a/test/es-module/test-cjs-legacyMainResolve-permission.js +++ b/test/es-module/test-cjs-legacyMainResolve-permission.js @@ -63,9 +63,11 @@ describe('legacyMainResolve', () => { assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { code: 'ERR_ACCESS_DENIED', - resource: path.resolve( - ${JSON.stringify(fixtextureFolderEscaped)}, - ${JSON.stringify(mainOrFolder)}, + resource: path.toNamespacedPath( + path.resolve( + ${JSON.stringify(fixtextureFolderEscaped)}, + ${JSON.stringify(mainOrFolder)}, + ) ) }); `, @@ -120,10 +122,12 @@ describe('legacyMainResolve', () => { assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { code: 'ERR_ACCESS_DENIED', - resource: path.resolve( - ${JSON.stringify(fixtextureFolderEscaped)}, - ${JSON.stringify(folder)}, - ${JSON.stringify(expectedFile)}, + resource: path.toNamespacedPath( + path.resolve( + ${JSON.stringify(fixtextureFolderEscaped)}, + ${JSON.stringify(folder)}, + ${JSON.stringify(expectedFile)}, + ) ) }); `, diff --git a/test/es-module/test-esm-long-path-win.js b/test/es-module/test-esm-long-path-win.js new file mode 100644 index 00000000000000..ef47759698992c --- /dev/null +++ b/test/es-module/test-esm-long-path-win.js @@ -0,0 +1,189 @@ +'use strict'; + +// Flags: --expose-internals + +const common = require('../common'); +if (!common.isWindows) { + common.skip('this test is Windows-specific.'); +} + +const fs = require('fs'); +const path = require('path'); +const tmpdir = require('../common/tmpdir'); +const { describe, it } = require('node:test'); +const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); +const { pathToFileURL } = require('node:url'); +const { spawnPromisified } = require('../common'); +const assert = require('assert'); +const { execPath } = require('node:process'); + +describe('long path on Windows', () => { + it('check long path in ReadPackageJSON', () => { + // Module layout will be the following: + // package.json + // main + // main.js + const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); + const packageDirName = tmpdir.resolve('x'.repeat(packageDirNameLen)); + const packageDirPath = path.resolve(packageDirName); + const packageJsonFilePath = path.join(packageDirPath, 'package.json'); + const mainDirName = 'main'; + const mainDirPath = path.resolve(packageDirPath, mainDirName); + const mainJsFile = 'main.js'; + const mainJsFilePath = path.resolve(mainDirPath, mainJsFile); + + tmpdir.refresh(); + + fs.mkdirSync(packageDirPath); + fs.writeFileSync( + packageJsonFilePath, + `{"main":"${mainDirName}/${mainJsFile}"}` + ); + fs.mkdirSync(mainDirPath); + fs.writeFileSync(mainJsFilePath, 'console.log("hello world");'); + + require('internal/modules/run_main').executeUserEntryPoint(packageDirPath); + + tmpdir.refresh(); + }); + + it('check long path in LegacyMainResolve - 1', () => { + // Module layout will be the following: + // package.json + // index.js + const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); + const packageDirPath = tmpdir.resolve('y'.repeat(packageDirNameLen)); + const packageJSPath = path.join(packageDirPath, 'package.json'); + const indexJSPath = path.join(packageDirPath, 'index.js'); + const packageConfig = { }; + + tmpdir.refresh(); + + fs.mkdirSync(packageDirPath); + fs.writeFileSync(packageJSPath, ''); + fs.writeFileSync(indexJSPath, ''); + + const packageJsonUrl = pathToFileURL( + path.resolve(packageJSPath) + ); + + console.log(legacyMainResolve(packageJsonUrl, packageConfig, '')); + }); + + it('check long path in LegacyMainResolve - 2', () => { + // Module layout will be the following: + // package.json + // main.js + const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); + const packageDirPath = tmpdir.resolve('z'.repeat(packageDirNameLen)); + const packageJSPath = path.join(packageDirPath, 'package.json'); + const indexJSPath = path.join(packageDirPath, 'main.js'); + const packageConfig = { main: 'main.js' }; + + tmpdir.refresh(); + + fs.mkdirSync(packageDirPath); + fs.writeFileSync(packageJSPath, ''); + fs.writeFileSync(indexJSPath, ''); + + const packageJsonUrl = pathToFileURL( + path.resolve(packageJSPath) + ); + + console.log(legacyMainResolve(packageJsonUrl, packageConfig, '')); + }); + + it('check long path in GetNearestParentPackageJSON', async () => { + // Module layout will be the following: + // node_modules + // test-module + // package.json (path is less than 260 chars long) + // cjs + // package.json (path is more than 260 chars long) + // index.js + const testModuleDirPath = 'node_modules/test-module'; + let cjsDirPath = path.join(testModuleDirPath, 'cjs'); + let modulePackageJSPath = path.join(testModuleDirPath, 'package.json'); + let cjsPackageJSPath = path.join(cjsDirPath, 'package.json'); + let cjsIndexJSPath = path.join(cjsDirPath, 'index.js'); + + const tmpDirNameLen = Math.max( + 260 - tmpdir.path.length - cjsPackageJSPath.length, 1); + const tmpDirPath = tmpdir.resolve('k'.repeat(tmpDirNameLen)); + + cjsDirPath = path.join(tmpDirPath, cjsDirPath); + modulePackageJSPath = path.join(tmpDirPath, modulePackageJSPath); + cjsPackageJSPath = path.join(tmpDirPath, cjsPackageJSPath); + cjsIndexJSPath = path.join(tmpDirPath, cjsIndexJSPath); + + tmpdir.refresh(); + + fs.mkdirSync(cjsDirPath, { recursive: true }); + fs.writeFileSync( + modulePackageJSPath, + `{ + "type": "module" + }` + ); + fs.writeFileSync( + cjsPackageJSPath, + `{ + "type": "commonjs" + }` + ); + fs.writeFileSync( + cjsIndexJSPath, + 'const fs = require("fs");' + ); + const { code, signal, stderr } = await spawnPromisified(execPath, [cjsIndexJSPath]); + assert.strictEqual(stderr.trim(), ''); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + }); + + it('check long path in GetNearestParentPackageJSONType', async () => { + // Module layout will be the following: + // node_modules + // test-module + // package.json (path is less than 260 chars long) + // cjs + // package.json (path is more than 260 chars long) + // index.js + const testModuleDirPath = 'node_modules/test-module'; + let cjsDirPath = path.join(testModuleDirPath, 'cjs'); + let modulePackageJSPath = path.join(testModuleDirPath, 'package.json'); + let cjsPackageJSPath = path.join(cjsDirPath, 'package.json'); + let cjsIndexJSPath = path.join(cjsDirPath, 'index.js'); + + const tmpDirNameLen = Math.max(260 - tmpdir.path.length - cjsPackageJSPath.length, 1); + const tmpDirPath = tmpdir.resolve('l'.repeat(tmpDirNameLen)); + + cjsDirPath = path.join(tmpDirPath, cjsDirPath); + modulePackageJSPath = path.join(tmpDirPath, modulePackageJSPath); + cjsPackageJSPath = path.join(tmpDirPath, cjsPackageJSPath); + cjsIndexJSPath = path.join(tmpDirPath, cjsIndexJSPath); + + tmpdir.refresh(); + + fs.mkdirSync(cjsDirPath, { recursive: true }); + fs.writeFileSync( + modulePackageJSPath, + `{ + "type": "module" + }` + ); + fs.writeFileSync( + cjsPackageJSPath, + `{ + "type": "commonjs" + }` + ); + + fs.writeFileSync(cjsIndexJSPath, 'import fs from "node:fs/promises";'); + const { code, signal, stderr } = await spawnPromisified(execPath, [cjsIndexJSPath]); + + assert.ok(stderr.includes('Warning: To load an ES module')); + assert.strictEqual(code, 1); + assert.strictEqual(signal, null); + }); +}); From dae0bfe00f5330ee095dba7a2a66a40686fa235c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= Date: Mon, 15 Jul 2024 17:09:51 +0300 Subject: [PATCH 2/3] fixup! module,win: fix long path resolve --- src/node_file.cc | 4 ++-- src/node_modules.cc | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 30e1fcbdde8ded..f71c0f3a1aa82c 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3098,7 +3098,7 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); Local local_file_path = Buffer::Copy( - env->isolate(), file_path.c_str(), strlen(file_path.c_str())) + env->isolate(), file_path.c_str(), file_path.size()) .ToLocalChecked(); BufferValue buff_file_path(isolate, local_file_path); ToNamespacedPath(env, &buff_file_path); @@ -3141,7 +3141,7 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); Local local_file_path = Buffer::Copy( - env->isolate(), file_path.c_str(), strlen(file_path.c_str())) + env->isolate(), file_path.c_str(), file_path.size()) .ToLocalChecked(); BufferValue buff_file_path(isolate, local_file_path); ToNamespacedPath(env, &buff_file_path); diff --git a/src/node_modules.cc b/src/node_modules.cc index 88252ccf7b8a2d..80f0855ba5ee16 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -323,8 +323,8 @@ void BindingData::GetNearestParentPackageJSON( // Check if the path has a trailing slash. If so, add it after // ToNamespacedPath() as it will be deleted by ToNamespacedPath() bool slashCheck = !path_value.ToString().empty() && - path_value.ToString().back() == - std::filesystem::path::preferred_separator; + path_value.ToStringView().ends_with( + std::filesystem::path::preferred_separator); ToNamespacedPath(realm->env(), &path_value); @@ -351,8 +351,8 @@ void BindingData::GetNearestParentPackageJSONType( // Check if the path has a trailing slash. If so, add it after // ToNamespacedPath() as it will be deleted by ToNamespacedPath() bool slashCheck = !path_value.ToString().empty() && - path_value.ToString().back() == - std::filesystem::path::preferred_separator; + path_value.ToStringView().ends_with( + std::filesystem::path::preferred_separator); ToNamespacedPath(realm->env(), &path_value); From 0e2c73986d4328341e50d1b2ab72197289b9f1ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= Date: Tue, 16 Jul 2024 13:33:28 +0300 Subject: [PATCH 3/3] fixup! module,win: fix long path resolve --- src/node_file.cc | 8 ++++---- src/node_modules.cc | 14 ++++++-------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index f71c0f3a1aa82c..abf1703862d0d2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3096,9 +3096,9 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { for (int i = 0; i < legacy_main_extensions_with_main_end; i++) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); + // TODO(anonrig): Remove this when ToNamespacedPath supports std::string Local local_file_path = - Buffer::Copy( - env->isolate(), file_path.c_str(), file_path.size()) + Buffer::Copy(env->isolate(), file_path.c_str(), file_path.size()) .ToLocalChecked(); BufferValue buff_file_path(isolate, local_file_path); ToNamespacedPath(env, &buff_file_path); @@ -3139,9 +3139,9 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { i < legacy_main_extensions_package_fallback_end; i++) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); + // TODO(anonrig): Remove this when ToNamespacedPath supports std::string Local local_file_path = - Buffer::Copy( - env->isolate(), file_path.c_str(), file_path.size()) + Buffer::Copy(env->isolate(), file_path.c_str(), file_path.size()) .ToLocalChecked(); BufferValue buff_file_path(isolate, local_file_path); ToNamespacedPath(env, &buff_file_path); diff --git a/src/node_modules.cc b/src/node_modules.cc index 80f0855ba5ee16..2649b07bf2cf13 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -263,8 +263,8 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo& args) { path.ToStringView()); ToNamespacedPath(realm->env(), &path); - auto package_json = - GetPackageJSON(realm, path.ToString(), is_esm ? &error_context : nullptr); + auto package_json = GetPackageJSON( + realm, path.ToStringView(), is_esm ? &error_context : nullptr); if (package_json == nullptr) { return; @@ -322,9 +322,8 @@ void BindingData::GetNearestParentPackageJSON( BufferValue path_value(realm->isolate(), args[0]); // Check if the path has a trailing slash. If so, add it after // ToNamespacedPath() as it will be deleted by ToNamespacedPath() - bool slashCheck = !path_value.ToString().empty() && - path_value.ToStringView().ends_with( - std::filesystem::path::preferred_separator); + bool slashCheck = path_value.ToStringView().ends_with( + std::filesystem::path::preferred_separator); ToNamespacedPath(realm->env(), &path_value); @@ -350,9 +349,8 @@ void BindingData::GetNearestParentPackageJSONType( BufferValue path_value(realm->isolate(), args[0]); // Check if the path has a trailing slash. If so, add it after // ToNamespacedPath() as it will be deleted by ToNamespacedPath() - bool slashCheck = !path_value.ToString().empty() && - path_value.ToStringView().ends_with( - std::filesystem::path::preferred_separator); + bool slashCheck = path_value.ToStringView().ends_with( + std::filesystem::path::preferred_separator); ToNamespacedPath(realm->env(), &path_value);