fs: fix long symlinks in realpath #7548

trevnorris · 2016-07-05T19:42:03Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

fs, uv

Description of change

fs: allow realpath to resolve deep symlinks

realpath(3) would fail if the symbolic link depth was too deep. If ELOOP
is encountered then resolve the path in parts until the entire thing is
resolved.

uv: expose uv_strerror() to JS API

Retrieving the error message along with the error name is useful. So
exposing to the JS API.

Also add additional -UV_ERRNO_MAX range check to ErrName() to help
prevent the following comment from libuv documentation on both
uv_strerror() and uv_err_name():

   Leaks a few bytes of memory when you call it with an unknown error
   code.

Fixes: #7175

This definitely isn't the prettiest code I've written. Want to get feedback while I finish up the tests.

R=@bnoordhuis
R=@saghul

mscdex · 2016-07-05T19:49:55Z

src/node_file.cc

+  size_t slash_count = 0;
+
+  while ((offset = str_path.find('/', offset + 1)) != std::string::npos) {
+    if (++slash_count < 32) {


Where does 32 come from?

From @saghul's comment in #7175 (comment).

I think having a comment here about why 32 was chosen might be helpful.

heh. true.

@saghul There a reason you mentioned 32 in your comment?

@trevnorris See #7175 (comment)

oops. missed that. thanks.

Trott · 2016-07-05T22:41:22Z

Possible/reasonable to add one or more tests?

Trott · 2016-07-05T22:41:50Z

/cc @jhamhader

trevnorris · 2016-07-05T22:48:37Z

@Trott

Possible/reasonable to add one or more tests?

Comment in OP :-)

Want to get feedback while I finish up the tests.

Trott · 2016-07-05T23:20:07Z

Heh...I got as far as "feedback" and stopped. Last mile problems.

saghul · 2016-07-06T22:12:40Z

src/node_file.cc

+static size_t CountSlashes(const char* str, size_t len) {
+  size_t cntr = 0;
+  for (size_t i = 0; i < len; i++) {
+    if (str[i] == '/') cntr++;


won't this be a problem on Windows? (I this code ever gets run there, that is)

I couldn't see in any of the win calls of uv_fs_realpath() that could return ERROR_CANT_RESOLVE_FILENAME (mapped to UV_ELOOP). Specifically those are CreateFileW() and pGetFinalPathNameByHandleW(). I just added a couple tests that will push up in a moment and run CI to check.

@saghul So, seems I can get ELOOP from windows. Working on that.

trevnorris · 2016-07-06T22:26:43Z

Updated and running CI: https://ci.nodejs.org/job/node-test-commit/3996/

trevnorris · 2016-07-06T23:16:18Z

Hm, the PR doesn't build properly on Windows. Looking into it.

On all the node-compile-windows there's:

src\node_file.cc(906): error C2660: 'uv_fs_realpath' : function does not take 3 arguments [c:\workspace\node-compile-windows\label\win-vs2013\node.vcxproj]

Not sure why that's failing for Windows and not any other box.

From node-test-binary-windows:

ERROR: Couldn't find any revision to build. Verify the repository and branch configuration for this job.

Not sure what that's about.

Fishrock123 · 2016-07-07T08:46:39Z

cc @nodejs/platform-windows ^^^

bzoz · 2016-07-07T10:27:53Z

If you change macro call SYNC_CALL_NO_THROW(req_wrap, realpath, path, path); to what it should expand to SYNC_DEST_CALL_NO_THROW(req_wrap, realpath, nullptr, path, path); it compiles under vs2015. I'll investigate what is happening there.

bzoz · 2016-07-07T10:51:55Z

It's a thing with how msvc expands __VA_ARGS__ (stackoverlow discussion). There is a workaround for this, by changing SYNC_CALL_NO_THROW definition:

#define EXPAND(X) X
#define SYNC_CALL_NO_THROW(func, path, ...)                                      \
  EXPAND(SYNC_DEST_CALL_NO_THROW(func, path, nullptr, __VA_ARGS__))

trevnorris · 2016-07-07T17:31:34Z

@bzoz Thanks much for looking into this. Does make me curious why the use of ASYNC_CALL and ASYNC_DEST_CALL don't suffer the same issue. Oh well. Trying out your fix now.

EDIT: Ah yup. I see it now. Thanks for pointing this out. Missed that the macros could be simplified, which will solve the issue.

trevnorris · 2016-07-07T18:53:55Z

Realized two things. 1) this doesn't address the async call (small oversight...) and 2) it can be made more reliable than it was before. Working on both.

trevnorris · 2016-07-07T20:22:20Z

Updated w/ most of it, but going to forget the "more reliable" part for now. That's an enhancement for another PR.

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

trevnorris · 2016-07-07T20:55:50Z

Compiling but now getting an issue with the resolved path on windows. Looking into it.

EDIT: Looks like it may be from exceeding MAX_PATH. Updating test and resubmitting.

trevnorris · 2016-07-07T21:06:01Z

Path length fixed.

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

addaleax · 2016-07-07T21:12:27Z

lib/fs.js

+    req.oncomplete = pathResolver;
+    binding.realpath(pathModule._makeLong(tmpPath), options.encoding, req);
+  }(null, ''));
+}


Would it make more sense to do a single async call to ResolveRealPath instead of mirroring the C++ code in JS?

I agree. Reason I did it this way was because handling/passing the async resources was being a thorn at the time. I'll see about moving this into C++.

trevnorris · 2016-07-08T17:47:53Z

@nodejs/platform-windows ~~So I can't get the test to repro properly on my VM. Could it have to do with the path that Jenkins is giving it?~~ /cc @rvagg

Nm. Just noticed that it didn't fail b/c the test wasn't actually running... Can repro.

trevnorris · 2016-07-08T22:54:57Z

Think I got it this time.

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

Fishrock123 · 2016-07-11T13:57:26Z

@trevnorris Lots of failures. :/

trevnorris · 2016-07-11T20:43:37Z

@Fishrock123 Yeah. But Windows passed! Not sure if that's progress though...

trevnorris · 2016-07-11T21:09:08Z

Another CI to help me understand what's going on: https://ci.nodejs.org/job/node-test-pull-request/3248/

trevnorris · 2016-07-11T21:17:42Z

@addaleax Heh. I'm somewhere between JS and C++ here. Thanks for the reminder what I'm actually coding.

trevnorris · 2016-07-11T21:24:00Z

Here we go again. CI: https://ci.nodejs.org/job/node-test-pull-request/3250/

addaleax · 2016-07-11T21:26:44Z

Btw… is there any reason for the path delimiter to be on env?

trevnorris · 2016-07-11T21:28:59Z

@addaleax didn't want to run strcmp() every time, and I've been trained by @bnoordhuis to never use globals. So, psychological conditioning would be the reason.

addaleax · 2016-07-11T21:31:28Z

Just wondering if a #ifdef _WIN32-based macro wouldn’t be cheaper, the value does seem pretty constant to me. ;)

trevnorris · 2016-07-11T21:38:15Z

Hah. That would in fact be better C++. Again failed on the context switch from a temporary implementation in JS using process.platform. Thanks. =)

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

trevnorris · 2016-07-11T22:50:07Z

Yey! No related failures. Will clean up commits.

Retrieving the error message along with the error name is useful. So exposing to the JS API. Extend the existing error messages to include more information about the invalid error code. Also add additional UV_ERRNO_MAX range check to ErrName() to help prevent the following comment from libuv documentation on both uv_strerror() and uv_err_name(): Leaks a few bytes of memory when you call it with an unknown error code.

realpath(3) would fail if the symbolic link depth was too deep. If ELOOP is encountered then resolve the path in parts until the entire thing is resolved. This excludes if the number of symbolic links is too deep, or if they are recursive. Fixes: nodejs#7175

The test/benchmarks included also work for the previous JS implementation of fs.realpath(). In case the new implementation of realpath() needs to be reverted, we want these changes to stick around.

jasnell · 2016-08-08T19:23:23Z

@trevnorris ... should this one remain open given that it looks like we're reverting the impl back to the old js impl?

trevnorris · 2016-08-09T16:48:37Z

@jasnell well, if it's going to take a full major release cycle before libuv is fixed and landed then yeah. probably. though i'd like to hope that it won't.

jasnell · 2016-08-09T17:21:51Z

Marking this as blocked for now then

Fishrock123 · 2016-10-14T20:41:50Z

This has since been fixed by reverting to js realpath, closing.

@trevnorris please let me know if closing this was in error

trevnorris added the fs Issues and PRs related to the fs subsystem / file system. label Jul 5, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 5, 2016

mscdex reviewed Jul 5, 2016
View reviewed changes

trevnorris force-pushed the realpath-fix branch from a41a2c1 to 80531bf Compare July 5, 2016 22:42

Fishrock123 mentioned this pull request Jul 6, 2016

win, fs: fix realpath behavior on substed drives #7559

Closed

3 tasks

trevnorris force-pushed the realpath-fix branch from 80531bf to 3572ec9 Compare July 6, 2016 19:25

saghul reviewed Jul 6, 2016
View reviewed changes

trevnorris force-pushed the realpath-fix branch from 3572ec9 to ca9416e Compare July 6, 2016 22:22

trevnorris force-pushed the realpath-fix branch from ca9416e to 3d9b8f8 Compare July 7, 2016 20:19

trevnorris force-pushed the realpath-fix branch from 3d9b8f8 to 2f2dd13 Compare July 7, 2016 21:05

addaleax reviewed Jul 7, 2016
View reviewed changes

trevnorris force-pushed the realpath-fix branch from 2f2dd13 to 5274190 Compare July 8, 2016 17:28

trevnorris force-pushed the realpath-fix branch from c81418b to 31ccb61 Compare July 8, 2016 22:54

trevnorris force-pushed the realpath-fix branch from 5243be1 to a9ad170 Compare July 11, 2016 21:22

trevnorris force-pushed the realpath-fix branch from a9ad170 to 4dc7a63 Compare July 11, 2016 21:40

trevnorris added 3 commits July 14, 2016 11:52

test,bench: add tests/bench for fs.realpath() fix

e7a38ab

The test/benchmarks included also work for the previous JS implementation of fs.realpath(). In case the new implementation of realpath() needs to be reverted, we want these changes to stick around.

trevnorris force-pushed the realpath-fix branch from 4dc7a63 to e7a38ab Compare July 14, 2016 21:10

addaleax mentioned this pull request Jul 20, 2016

[meta] realpath issues in v6 #7726

Closed

7 tasks

addaleax mentioned this pull request Jul 27, 2016

fs: restore javascript realpath implementation #7899

Closed

2 tasks

jasnell added the blocked PRs that are blocked by other issues or PRs. label Aug 9, 2016

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

Fishrock123 closed this Oct 14, 2016

fs: fix long symlinks in realpath #7548

fs: fix long symlinks in realpath #7548

Conversation

trevnorris commented Jul 5, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jul 5, 2016

Trott commented Jul 5, 2016

trevnorris commented Jul 5, 2016

Trott commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jul 6, 2016

trevnorris commented Jul 6, 2016

Fishrock123 commented Jul 7, 2016

bzoz commented Jul 7, 2016

bzoz commented Jul 7, 2016

trevnorris commented Jul 7, 2016 • edited Loading

trevnorris commented Jul 7, 2016

trevnorris commented Jul 7, 2016

trevnorris commented Jul 7, 2016 • edited Loading

trevnorris commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jul 8, 2016 • edited Loading

trevnorris commented Jul 8, 2016

Fishrock123 commented Jul 11, 2016

trevnorris commented Jul 11, 2016

trevnorris commented Jul 11, 2016

trevnorris commented Jul 11, 2016

trevnorris commented Jul 11, 2016

addaleax commented Jul 11, 2016

trevnorris commented Jul 11, 2016

addaleax commented Jul 11, 2016

trevnorris commented Jul 11, 2016 • edited Loading

trevnorris commented Jul 11, 2016

jasnell commented Aug 8, 2016

trevnorris commented Aug 9, 2016

jasnell commented Aug 9, 2016

Fishrock123 commented Oct 14, 2016

trevnorris commented Jul 7, 2016 •

edited

Loading

trevnorris commented Jul 7, 2016 •

edited

Loading

trevnorris commented Jul 8, 2016 •

edited

Loading

trevnorris commented Jul 11, 2016 •

edited

Loading