process.chdir is not thread-safe #618

Closed
piscisaureus opened this Issue Feb 1, 2011 · 6 comments

Comments

Projects
None yet
5 participants
Member

piscisaureus commented Feb 1, 2011

Calling process.chdir() while libeio is processing fs operations that require resolving a relative pathname to an absolute one could be dangerous because chdir() may not be thread-safe, so a new CWD could be partially applied when a relative path is resolved in a thread.

Apart from that, it yields unpredictable result, because you can never know which cwd is used in a fs operation that is pending while chdir is called.

Consensus solution is to absolute-ize paths before they hit the thread pool. It could be done by calling path.resolve() in fs.js before invoking the binding.

Known affected: open, symlink, mkdir, maybe more

A test is here: https://gist.github.com/805263#file_test.js

This is assigned to piscisaureus. Remind him if he forgets.

piscisaureus was assigned Aug 10, 2011

@piscisaureus Just ran your gist test against v0.8.14 and everything passed. Does that mean this issue is resolved?

Owner

bnoordhuis commented Dec 5, 2012

It's the world upside down: it turns out that the issue is fixed on Windows but not on UNIX.

It's trivial to fix (see patch below) but it breaks tests that expect relative paths in error messages, like simple/test-domain.

diff --git a/lib/path.js b/lib/path.js
index 139d04a..d6f33dd 100644
--- a/lib/path.js
+++ b/lib/path.js
@@ -469,7 +469,5 @@ if (isWindows) {
     return path;
   };
 } else {
-  exports._makeLong = function(path) {
-    return path;
-  };
+  exports._makeLong = exports.resolve;
 }

@bnoordhuis bnoordhuis added a commit to bnoordhuis/node that referenced this issue Dec 5, 2012

@bnoordhuis bnoordhuis fs: make file paths absolute before operation
Make file paths absolute before starting the fs operation. Avoids chdir() races
like in the test case below:

  // test.js
  for (var i = 0; i < 10; i++) {
    require('fs').open('test.js', function(err, fd) {
      if (err) throw err;
    });
  }
  process.chdir('/');

Before this commit, such code would sometimes fail because the chdir() is
executed inside the main thread while the open() operations run concurrently
in the thread pool.

Fixes #618.
3cee989
Owner

bnoordhuis commented Dec 5, 2012

Can someone review 3cee989?

@piscisaureus can you confirm the state of this issue? is it still a thing?

Owner

jasnell commented Dec 12, 2014

fwiw... testing this on mac against master (v0.13.0-pre) shows the given test case passing.

Owner

jasnell commented May 15, 2015

Going to assume that since the test is passing this isn't an issue any longer. Closing.

jasnell closed this May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment