win, fs: fix realpath behavior on substed drives

[bzoz]

Checklist

• make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
• benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

fs, win

Description of change

Work in progress for issues related to new realpath implementation (#7175).

Before v6 on drives created with subst and network mapped drives realpath used to return filename on the mapped drive. With v6 (b488b19) it now returns filename on the original device or on the network share. This restores the old behavior by:

1. Restoring old javascript implementation (separate commit just for reviewing here, can be squashed with the next)
2. Using it when path returned by new implementation is on a different device than the original path

This PR addresses only issues related to the filename resolved being different on v6 and v4 (e.g. #7294). There are other issues, for example related to permissions on Windows like #7192 and other on Linux as described in #7175. Some of those are to be addressed in different patch (comment).

@trevnorris this fixes some of the Windows issues. Can you review to make sure it's compatible with your changes?

[Fishrock123]

#7548 Should make this unnecessary?

[bzoz]

No, #7294 is different beast than ELOOP.

[addaleax]

One global realpath cache without some kind of cache validation is probably a no-go?

[bzoz]

That's a thing that needs to be solved. One global cache is probably not a good idea. But some caching is needed for performance reasons.

I would guess user could add cache object to options. It could be also used on Linux, to cache realpath results (not the partial ones as in JS implementation)

[mscdex]

It seems like this could be handled better in C++ instead of resorting to reinstating the js realpath implementation?

[Trott]

Labelling as in progress. Of course, if that's no longer the case, please comment or remove the label.

[jasnell]

Would definitely prefer to see if there's a fix that can be made in libuv before bringing back the js realpath impl.

[bzoz]

@mscdex, @jasnell: old JS implementation didn't check if the drive itself was not a "link" - e.g. subst or mapped network location. It just assumed its a hard drive, only checking if it exists. I don't think there is a workaround for this in uv - it calls GetFinalPathNameByHandle, and that will always return original drive/network share name and not the mapped one.

One thing that could be done, is to port old js implementation to C++, as a fallback for this corner case, but that would not be an easy task.

[mscdex]

@bzoz What I had in mind was to check if the drive is mapped when the input path starts with a drive letter that is different than that of the output path. If it is, then simply replace the beginning, common portion of the input and output paths with the drive letter from the input path.

[addaleax]

I think the other Windows issues that were arising from the v6 realpath change are a lot easier to fix when building upon this, likely by simple additions to realpathJSNeeded; so for now I’m +1 on this approach.

I’d also be +1 on moving the entire old implementation to C++, but as @bzoz said, that’s something for a different time scale.

[bzoz]

@mscdex: that would unfortunately not work:

• x: is mapped drive
• x:\test\ is a link to c:\test\
• we want realpath of x:\test\file.txt

Old JS would follow the link to C drive, returning c:\test\file.txt, just like native implementation does it now.

[mscdex]

@bzoz I'm confused now. I thought you said the C++ realpath and js realpath were returning different paths, so why would both return c:\test\file.txt?

[bzoz]

@mscdex: I was referring to this idea, of replacing common parts of paths. With this solution in my example x:\test\file.txt would be returned which would be incorrect.

[bzoz]

Maybe to clarify: return value of realpathSync('x:\test\file.txt)`:

No	What is x:	What is x:\test\	JS version	uv version
1	physical drive	mklink /d x:\test c:\test	c:\test\file.txt	c:\test\file.txt
2	physical drive	mkdir x:\test	x:\test\file.txt	x:\test\file.txt
3	subst x: c:\substed	mklink /d c:\substed c:\test	c:\substed\file.txt	c:\substed\file.txt
4	subst x: c:\substed	mkidr x:\test	x:\test\file.txt	c:\substed\test\file.txt

If we would use only uv version and try to replace the beginning, we would have a bug in case No 3, as it would get us x:\test\file.txt, when both new and old agree it should be c:\substed\file.txt. Hope this helps.

[trevnorris]

This does step on my realpath() fix. Also I feel like this is a little too much copy/paste when I see comments like:

// On windows, check that the root exists. On unix there is no need.

even though this PR only addresses Windows specific behavior. Would it not be possible to resolve the path normally then pass it to a function for fixup afterwards, instead of adding the entire JS implementation again?

[addaleax]

@trevnorris #6861 #7044 #7192 #7294 are a couple of Windows issues that were arising from the libuv-based realpath implementation, and I think your PR is orthogonal to fixing them?

[mscdex]

I think this can be addressed in C++ land by passing the original drive letter to QueryDosDevice(). If that function returns a path starting with \??\ then it's a subst'ed drive and we can simply do the appropriate string manipulation to arrive at the pre-node-v6 result.

[bzoz]

@trevnorris: JS realpath implementation is copy/pasted from before the change to uv. I didn't want to change it so it would be easier to review rest of the changes. But yes, I'll clean it up, remove those if (isWindows) etc.

What I try to do here is pretty much resolving the path normally using the new native realpath, and then running "fixup" if needed (if (realpathJSNeeded(...)). Unfortunately, the only fixup function that I come up with was the old implementation. The idea @mscdex has would be great, but there are corner cases. I can think of one, No 3 from the table above, but maybe there are more.

@addaleax: From the issues you mentioned my PR will only fix #7294, but as you noticed earlier we could extend this fix for the other issues that you mentioned.

@mscdex: Sorry, I just don't see how it could work. As I understand your idea, with No 3:

1. uv would return c:\substed\file.txt
2. we check that it is on different drive than x:\test\file.txt, so we need to fix
3. "common part" is file.txt, so we would use x:\test\ + file.txt
   The problem here is that pre-v6 implementation would return c:\substed\file.txt

[mscdex]

@bzoz Normal junctions can be detected as well.

[bzoz]

@mscdex I just cannot see how it could be done other than reimplementing JS code in C++. We would have to check every path segment to see if it is not a link.

[trevnorris]

@addaleax

I think your PR is orthogonal to fixing them?

It is, but also I don't believe re-adding the entire JS implementation is necessary to solve this issue.

@bzoz
It seems the key issue here with the new implementation is how it would previously "Skip over roots" (comment in start(), in fs.realpathSync()). Specifically when it came to network drives. So taking @mscdex's suggestion, if a network path such as \\servername\share$\... were returned then the common path would be taken. This removes the issue that you've outlined in (3). What cases wouldn't this cover?

In my initial implementation I actually was implementing a way of resolving arbitrarily long nested symlinks using uv_fs_readlink(). So basically a minimal version of the JS implementation. If we absolutely must do this, I think it would be advantageous to do this in C++ and reap greater benefits.

[bzoz]

I'll try to elaborate on 3: realpathSync('x:\dir_a\dir_b\file');. The x: is substed to c:\x_drive\.

If the dir_a and dir_b are normal directories C++ will return c:\x_drive\dir_a\dir_b\file. We then check that x:\ is c:\x_drive, substitute and get x:\dir_a\dir_b\file - just as pre-v6 did. That is case 4.

Case 3 is when dir_a is a link to c:\x_drive\dir_b. Then C++ will return c:\x_drive\dir_b\file - just as pre-v6. If we now substitute c:\x_drive\ we will get x:\dir_b\file which is wrong.

We could add some conditions to make it work. For example if we take x:\dir_b\file and change x: to c:\x_drive\ we get different path than the original. So we could assume that there was a link.

But when dir_a is a link to x:\dir_b: then C++ will return c:\x_drive\dir_b\file and pre-v6 should return x:\dir_b\file.

Now, we have x:\dir_a\dir_b\file as input, C++ returns c:\x_drive\dir_b\file and we need to return c:\x_drive\dir_b\file or x:\dir_b\file, depending on what dir_a is. There is no other way to do this, than checking each path segment - just like old JS code.

As for performance - isn't disk access the bottleneck anyway? Would it really be faster in C++? Also, here we have a working implementation, with caching etc. Sure, we could reimplement it in C++, but I don't think it would be easy to do so.

[Fishrock123]

Does make lint not catch this?

[saghul]

IMHO "fixing" case 3 is not correct. It might be how pre-v6 behave, but that doesn't make it correct. I guess it boils down to how we want to interpret drive subst-ing. In my mind it's like a symlink from /c/xthing to /x/ so since symlinks should be resolved by realpath, we shouldn't put the substed drive letter, but keep the resolved one.

As a general note: it might be better to have a meta-issue with the realpath edge cases that need fixing, there are other reasons why might want / need to go back to the JS implementation than this specific one: ELOOP, ramdisks, etc.

[bzoz]

I also think that "case 3" was pretty much a bug in pre-v6, but from discussion at #7175 I understand, that v6 change didn't suppose to change the way realpath work.

[saghul]

Please see #7726.

Saúl Ibarra Corretgé
http://bettercallsaghul.com

[bzoz]

This will be addressed when #7899 lands.