GeezerVison

Review requested:

• @nodejs/loaders
• @nodejs/modules

In case CI needs me to re-approve after your push (sometimes it does, sometimes it doesn't 🤪)

@JakobJingleheimer I think this issue still exists. Ref: nodejs/node-core-utils#677

Something we should discuss is whether the fallback behavior in the original getCwdSafe is a good idea. For example, look at the call site in the CommonJS loader:

    const parentPath = parent?.filename ?? getCwdSafe();
    const pkg = readPackageScope(parentPath) || {};

What readPackageScope does with this input is search for a package.json file in the current folder, then in its parent, then onward up the tree until it either reaches the root or is told that it’s not allowed to proceed any higher by a permissions check. So the behavior that his PR adds to the CommonJS loader here is a tiny security hole: by deleting the CWD after the app begins, that permissions check is bypassed and Node tries to read /package.json from the volume root. (Though if an application can go so far as delete the folder from which the application was launched, presumably they already have permissions to read a file from the root.)

What if instead we cached the CWD on application startup and just always returned that? Then there would be no need for the fallback or the try/catch; the string or URL instance could always be returned. The call site would need to decide how or if it wanted to handle the possibility that the path had been deleted since server startup; maybe it would decide to use the root of the volume instead, but presumably some of the time it could just return a default value (like the readPackageScope case, where it could return whatever would be returned if no package.json files were found even when searching up to the root) or it could error. But that should probably be a case-by-case decision rather than something abstracted in a helper.

Would process.chdir still alter the cached value?

My goal when creating getSafeCwd() was to minimize the code, removing try/catch blocks that make things a bit difficult to read and also reuse code by abstracting it in a helper.

I agree that it should be a case-by-case decision, but we have to pay attention to the fact that I'm not using getSafeCwd() in all places (trying to replace process.cwd() everywhere), the goal was to use it only where we got the
try/catch blocks.

So maybe I should remove it from:

https://github.com/nodejs/node/blob/eb3702287b999e2ea71f71ca52c5c47d960bbd07/lib/internal/modules/cjs/loader.js#L1029

And:

https://github.com/nodejs/node/blob/eb3702287b999e2ea71f71ca52c5c47d960bbd07/lib/internal/modules/esm/resolve.js#L1022

Also, IMO, renaming it to something like getSafeCwdURL() and return an instance of a URL is a very nice approach, since we are always using pathToFileURL(getSafeCwd()).href everywhere.

I’m not using getSafeCwd() in all places (trying to replace process.cwd() everywhere), the goal was to use it only where we got the
try/catch blocks.

The one in cjs/loader.js isn’t using a try/catch, so I’d revert that change. It’s also the only one expecting a path rather than a URL, so it could be left for another PR.

Since all the others expect a URL, you could make the helper function return a URL. Looking a little closer, all three of the remaining call sites are using the helper to define parentURL; and file:/// isn’t actually correct (that’s not a module’s parent’s URL) so arguably returning / or the Windows equivalent isn’t really what we should be doing for our fallback. I think arguably the most correct behavior in these cases is probably to return what the CWD was, since I think these are all referencing the main entry point of the application and at the time, the CWD existed. So we could create a helper like this:

const cwdAtStartup = pathToFileURL(process.cwd());

function getCwdAtStartup() {
  return cwdAtStartup;
}

And I don’t think we need a try around this because this code would run on startup before the application logic had a chance to delete the CWD. And I would add the trailing slash at the calling sites rather than include it the return value of the helper, for consistency with process.cwd.

So then the question becomes, does anything break if we define a parentURL to be a path that doesn’t exist? And if so, I guess that’s where we would need to put try/catch blocks, and in those cases the catch should be something more specific to the context of that part of the code (return a default value, error, etc.).

What do others think?

And I don’t think we need a try around this because this code would run on startup before the application logic had a chance to delete the CWD.

The CWD can be deleted by something that's not the application, in all likelihood node is not the only process running. It's a flawed reasoning to only take node into account, it's still a race condition. I'm not saying we should try/catch it, probably it's fine if it fails at this point, but it would be wrong to assume it can't throw.

Note that process.cwd() already returns a cached value (well only if node owns its own state), see:

I think we need to create a util that looks like

let cachedURL;
let cachedCWD;
function getCWDURL() {
  let cwd;
  try {
    cwd = process.cwd() + sep;
  } catch {}
  if (cwd != null && cwd !== cachedCWD) {
    cachedURL = pathToFileURL(cwd);
    cachedCWD = cwd;
  }
  return cachedURL;
}

I think that would take care of all the edge cases, while avoid re-creating URL instances. wdyt?

Note that process.cwd() already returns a cached value

If that’s the case, why/when would it ever throw? It tries to validate the path’s existence even though it’s been cached? So another solution could be process.cwd({ validate: false }) to disable the check and just return the cached value?

Note that process.cwd() already returns a cached value

If that’s the case, why/when would it ever throw? It tries to validate the path’s existence even though it’s been cached? So another solution could be process.cwd({ validate: false }) to disable the check and just return the cached value?

No it always return the cached value when there's one, there's no validation, look at the code I shared above. There's no cached value when node doesn't own its own state (that's a build flag).

No it always return the cached value when there’s one

I’m asking about process.cwd, not your proposed new function. If process.cwd returns a cached value, what are the circumstances in which it would throw? Leaving aside this alternate build mode that Node could be built as; assuming that the “does own process state” mode is the default, when can process.cwd throw? The tests involve deleting the CWD path before calling process.cwd(), but why would that cause an exception if a cached value was returned?

I'm not sure what to do to make myself clear, I'm soon going to run out of code snippets to share 😅

Wether if node is build with kOwnsProcessState build flag (which is included with kDefaultFlags):

it's going to affect which file between internal/bootstrap/switches/does_own_process_stat or internal/bootstrap/switches/does_not_own_process_stat gets included in the binary

In the default case (i.e. all the official binaries), node own its own state, meaning it can meaningfully cache the value of process.cwd() (that's the wrappedCwd function, if you run node -p process.cwd.name you should see wrappedCwd) and it can't throw except at the first invocation.

In the case where node is built by an embedder (i.e. Electron and the likes) that has turned off the built flag, node doesn't own its own state, so we can't cache and the raw libuv function (the one that can throw) is used every time.

TL;DR process.cwd() is already as optimised as it can be, but still it can throw in some rare edge cases, and I think the implementation I suggested in #48434 (comment) is our best shot.

I'd like to point out that I'm not an expert in process.cwd, I got all the above information from reading the code, which I encourage you to do, because while I agree it's a good thing to brainstorm optimisation ideas, the conversation would be a lot more productive if I wasn't the only one checking if they actually make sense 🙈 I have the feeling that you make me work extra time to explain to you our codebase, and I think it's not fair, you should either accept my conclusion and trust it to be good, or do the actual work of verifying it (FWIW I'd prefer the latter, I'm always happy to be proven wrong). I'm under the impression that my options in this thread is either to keep drafting detailed comments and getting not very informed questions the next day as a response, or to stop participating. Anyway, sorry for my rant, I'll retire from this thread now.

To me, there are two issues here:

1. If process.cwd is caching its initial value (for default builds) then I would think that it should never throw. If it still does, that feels like a bug. This can be investigated in a separate PR, if it’s something we feel like dealing with.

2. The current call sites of getCwdSafe all seem to be able to survive with an invalid/not-very-sensical value for parentURL returned by the current getCwdSafe. I think the best solution of all would be to see if these call sites can be refactored to behave sensibly if parentURL is undefined. Then we don’t need getCwdSafe at all and can get rid of it. Each call site could look like this:

let parentURL;
try {
  parentURL = pathToFileURL(cwd() + sep);
} catch {
  /* Sometimes cwd() throws, in which case parentURL remains undefined */
}

// Following code should be able to handle a possibly undefined parentURL

This can be abstracted if desired, but the same goal would apply: rather than returning the root of the volume as the parent URL when CWD cannot be determined, just return undefined; and the code using that return value should be able to continue working when that return value is undefined. Is such a refactoring possible @jlenon7?

Alternatively, if it’s required that parentURL always be defined (either because we can’t make the code work otherwise or because there’s some spec/UX reason that it should always be defined, like so that we always pass in a defined value to custom loader hooks) then the question becomes what should it be when CWD is unknown. Should it be a cached value of CWD, and if so, why? Should it be the root of the volume, and if so, why? Once we answer that question, we can create an abstracted function like getParentURL to define the logic, based on one of the proposed getCwdSafe versions proposed above.

People, sorry for disappearing 😅. I was moving to Lisbon. Now things are much calmer and I can get back with this PR.

I'm going to follow the implementation that @aduh95 mentioned, wdyt?

I think we need to create a util that looks like

let cachedURL;
let cachedCWD;
function getCWDURL() {
  let cwd;
  try {
    cwd = process.cwd() + sep;
  } catch {}
  if (cwd != null && cwd !== cachedCWD) {
    cachedURL = pathToFileURL(cwd);
  }
  return cachedURL;
}

I think that would take care of all the edge cases, while avoid re-creating URL instances. wdyt?

CI: https://ci.nodejs.org/job/node-test-pull-request/54142/

CI: https://ci.nodejs.org/job/node-test-pull-request/54144/

Landed in c2cd744