fs: add uv_fs_realpath()#531

jhamhader · 2015-09-18T18:00:00Z

Add uv_fs_realpath() - returns the full resolved absolute path of a
file or directory.

thefourtheye · 2015-09-18T18:06:52Z

If I understand realpath docs properly, if we pass NULL as the second parameter, it will take care of the memory allocation and return the path. In fact, the patch which I wanted to submit had just this

static ssize_t uv__fs_realpath(uv_fs_t* req) { req->ptr = realpath(req->path, NULL); return req->ptr == NULL ? -1 : 0; }

I am not sure if it is correct though.

True, realpath() does that, however, I thought it would be better to allocate using uv__malloc() - for profiling (valgrind?) and we free that memory using its counterpart uv__free().

Also, sorry for white-boxing - seems like at least glibc allocates PATH_MAX as well, so no memory is spared when calling realpath with NULL :)

That looks ok to me, Yuval.

On Friday, September 18, 2015, Yuval Brik notifications@github.com wrote:

In src/unix/fs.c
#531 (comment):

if (len == -1) {
+#if defined(PATH_MAX)

len = PATH_MAX;
+#else

len = 4096;
+#endif

}

buf = uv__malloc(len + 1);

if (buf == NULL) {

errno = ENOMEM;

return -1;

}

if (realpath(req->path, buf) == NULL) {

True, realpath() does that, but I thought it would be better to allocate
using uv__malloc() - for profiling and valgrind. Also, as we freeing that
memory using its counterpart uv__free().

—
Reply to this email directly or view it on GitHub
https://github.com/libuv/libuv/pull/531/files#r39887331.

/Saúl
bettercallsaghul.com

POSIX pedantry: If resolved_name is a null pointer, the behavior of realpath() is implementation-defined.

On older versions of OS X, for example, passing in NULL segfaults.

A bit late note:

@bnoordhuis

POSIX pedantry: If resolved_name is a null pointer, the behavior of realpath() is implementation-defined.

That was correct for older POSIX versions only, in the current version passing a NULL pointer as the resolved_name is properly documented: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html

While true, it's irrelevant until we drop support for OS X 10.6 (where passing NULL segfaults.)

drop support for OS X 10.6

Ref: nodejs/node#5731

Also, #758 suggests dropping OS X 10.6 support in libuv.

bnoordhuis · 2015-09-18T20:37:45Z

Tests look good to me.

saghul · 2015-09-21T21:43:30Z

Add a ".. versionadded:: 1.8.0" below.

saghul · 2015-09-21T21:47:06Z

Left some minor comments.

jhamhader · 2015-09-23T07:47:42Z

Added the Windows implementation (using GetFinalPathNameByHandleW())
Refactored out a common readlink and realpath code into fs__wide_to_utf8()
Fixed tests
@saghul: added versionadded, fixed styling, realpath(3)

saghul · 2015-09-23T08:02:07Z

Leave a newline here.

saghul · 2015-09-23T08:07:35Z

I just took a quick peek.

Added the Windows implementation (using GetFinalPathNameByHandleW())

This is going to be a problem, since it's only supported in Windows >= Vista, and we still support XP. Now, since XP doesn't support symlinks (IIRC), maybe we can have some fallback path which basically copies the input into the output? (not sure if we'd need to do anything about specifying the drive). @piscisaureus WDYT?

piscisaureus · 2015-09-24T20:46:22Z

This is going to be a problem, since it's only supported in Windows >= Vista, and we still support XP. Now, since XP doesn't support symlinks (IIRC),

Windows XP does support junctions, which are treated by libuv as a type of symlink.

maybe we can have some fallback path which basically copies the input into the output?

That sounds like meh... maybe we could just return UV_ENOSYS on XP ?

saghul · 2015-09-24T20:48:30Z

That sounds like meh... maybe we could just return UV_ENOSYS on XP ?

I thought of it, but this PR originates from Node wanting to use it instead of the JavaScript implementation, so IMHO returning UV_ENOSYS would complicate the code there...

jhamhader · 2015-10-08T18:56:15Z

Not being highly familiar with Windows myself (a linux enthusiast), do you think we can somehow progress on this? Some ideas over Stack Overflow seem not very neat.
MSDN purposes using CreateFileMapping(), MapViewOfFile() and GetMappedFileName(), however - mapping of directories and zero-length files cannot be obtained.

Another issue I would like to raise: GetFinalPathNameByHandle() will return the full path with an upper-case drive letter. Is there a convention in libuv about that? I know node.js currently returns a lower-case drive letter.

jhamhader · 2015-10-18T19:06:55Z

@saghul @piscisaureus would really appreciate your opinion.

bnoordhuis · 2015-10-30T11:03:13Z

Probably worth noting that nodejs/node#3594 proposes to replace node's lovingly handcrafted fs.realpathSync() with a call to uv_fs_realpath(). fs.realpathSync() is used by the module loader so returning UV_ENOSYS on Windows XP would break node completely on that platform.

saghul · 2015-11-04T21:18:05Z

@bnoordhuis How about this: we return UV_ENOSYS on XP. Then Node caches that and uses the current lovingly crafted code as a fallback. Once v2 is a thing and XP support is gone the fallback can be removed and go to code heaven.

I really don't want anyone losing time here because of Windows XP...

bnoordhuis · 2015-11-04T21:51:41Z

I'm not in love with that because it means double the complexity and double the surface area for bugs and incompatibilities in node.js. I guess we can land this PR while leaving out the node.js changes but that kind of nullifies the goal of this PR.

saghul · 2015-12-04T12:49:32Z

Please mention what we use on Windows and the fact that it doesn't work on XP.

saghul · 2015-12-04T12:55:43Z

New CI: https://ci.nodejs.org/job/libuv+any-pr+multi/208/

Also left some comments.

saghul · 2015-12-04T13:06:14Z

Test pass, good job! Can you take care of the last issues a pointed out? Other than that I think this is ready to land.

jhamhader · 2015-12-04T17:17:46Z

Pushed. Can you review the phrasing of the Windows XP note?

saghul · 2015-12-04T17:40:21Z

Sorry to be nitpicky here... can you add that it returns UV_ENOSYS?

saghul · 2015-12-04T17:44:43Z

Some last comments, almost there!

Add uv_fs_realpath() - returns the full resolved absolute path of a file or directory.

jhamhader · 2015-12-04T20:16:26Z

Pushed. Please tell me if anything else is required.

saghul · 2015-12-04T21:26:32Z

LGTM, great work!

I'll run the CI once more and land it.
On Dec 4, 2015 9:16 PM, "Yuval Brik" notifications@github.com wrote:

Pushed. Please tell me if anything else is required.

—
Reply to this email directly or view it on GitHub
#531 (comment).

Equivalent to realpath(3), returns the full resolved absolute path of a file or directory. PR-URL: #531 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul · 2015-12-05T10:35:47Z

Thanks a lot Yuval, and great work! ✨ Landed in e76b883 ✨

fs.realpath has been optimized to utilize `ev_fs_realpath()`, a new api in libuv. The libuv api has slightly different behavior. There are two particular issues that are causing problems for glob * `ev_fs_realpath()` throwing before `fs.readdir` ELOOP * The removal of the cache In the past glob was relying on `fs.readdir` to throw when handling recursive symbolically linked directories. It is now possible that the call to libuv can throw before. The behavior is currently different on osx and linux causing the test suite to fail on all of the flavors of linux we are running in CI. This commit adds an extra check for 'ELOOP' and sets appropriately. There is an a check included to make sure that real ref: libuv/libuv#531 ref: nodejs/node#3594

fs.realpath has been optimized to utilize `ev_fs_realpath()`, a new api in libuv. The libuv api has slightly different behavior. There are two particular issues that are causing problems for glob * `ev_fs_realpath()` throwing before `fs.readdir` ELOOP * The removal of the cache In the past glob was relying on `fs.readdir` to throw when handling recursive symbolically linked directories. It is now possible that the call to libuv can throw before. The behavior is currently different on osx and linux causing the test suite to fail on all of the flavors of linux we are running in CI. This commit adds an extra check for 'ELOOP' and sets appropriately. The commit includes an extra check to make sure that `real` is truthy Without that extra check the test suite was reporting garbage data in the results. This was only happening with the async implementation. This commit has not done anything regarding the removal of the cache As long as the cache doesn't include a value called "encoding" everything will be fine. There has not been any benchmarking done on this change. ref: libuv/libuv#531 ref: nodejs/node#3594

fs.realpath has been optimized to utilize `uv_fs_realpath()`, a new api in libuv. The libuv api has slightly different behavior. There are two particular issues that are causing problems for glob * `ev_fs_realpath()` throwing before `fs.readdir` ELOOP * The removal of the cache In the past glob was relying on `fs.readdir` to throw when handling recursive symbolically linked directories. It is now possible that the call to libuv can throw before. The behavior is currently different on osx and linux causing the test suite to fail on all of the flavors of linux we are running in CI. This commit adds an extra check for 'ELOOP' and sets appropriately. The commit includes an extra check to make sure that `real` is truthy Without that extra check the test suite was reporting garbage data in the results. This was only happening with the async implementation. This commit has not done anything regarding the removal of the cache As long as the cache doesn't include a value called "encoding" everything will be fine. There has not been any benchmarking done on this change. ref: libuv/libuv#531 ref: nodejs/node#3594

MylesBorins · 2016-04-22T23:07:23Z

I've been digging into this as part of a security audit in the node project and came across a potential edge case in freebsd.

In freebsd limits.h does not contain PATH_MAX, it is found it syslimits.h and has the value of 1024

In an issue on the freebsd tracker it seems to imply that freebsd does not support longer path's out of the box. With this being the case I am wondering if setting the default to 4096 could potentially create a problem on freebsd.

fs.realpath has been optimized to utilize `uv_fs_realpath()`, a new api in libuv. The libuv api has slightly different behavior. There are two particular issues that are causing problems for glob * `ev_fs_realpath()` throwing before `fs.readdir` ELOOP * The removal of the cache In the past glob was relying on `fs.readdir` to throw when handling recursive symbolically linked directories. It is now possible that the call to libuv can throw before. The behavior is currently different on osx and linux causing the test suite to fail on all of the flavors of linux we are running in CI. This commit adds an extra check for 'ELOOP' and sets appropriately. The commit includes an extra check to make sure that `real` is truthy Without that extra check the test suite was reporting garbage data in the results. This was only happening with the async implementation. This commit has not done anything regarding the removal of the cache As long as the cache doesn't include a value called "encoding" everything will be fine. There has not been any benchmarking done on this change. ref: libuv/libuv#531 ref: nodejs/node#3594

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

anyone ever saw @thealphanerd's comment?

saghul · 2016-10-14T22:06:19Z

@Fishrock123 yep, I believe it landed, though for some reason this issue was not referenced: f617ccc

jhamhader mentioned this pull request Sep 18, 2015

fs.realpath 70x slower than native nodejs/node#2680

Closed

thefourtheye reviewed Sep 18, 2015
View reviewed changes

jhamhader force-pushed the realpath branch from 97a2378 to c165de6 Compare September 18, 2015 22:38

saghul reviewed Sep 21, 2015
View reviewed changes

Comment thread docs/src/fs.rst Outdated

Copy link
Copy Markdown

Member

saghul Sep 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a ".. versionadded:: 1.8.0" below.

jhamhader force-pushed the realpath branch 2 times, most recently from 3c4046f to 8829eff Compare September 23, 2015 07:37

saghul reviewed Sep 23, 2015
View reviewed changes

Comment thread docs/src/fs.rst Outdated

Copy link
Copy Markdown

Member

saghul Sep 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a newline here.

jhamhader force-pushed the realpath branch from 8829eff to b3509ac Compare September 23, 2015 08:30

jhamhader force-pushed the realpath branch 2 times, most recently from 3cadc57 to e7c76b6 Compare September 26, 2015 19:46

bnoordhuis mentioned this pull request Oct 30, 2015

fs: optimize realpath using uv_fs_realpath() nodejs/node#3594

Closed

jhamhader force-pushed the realpath branch from e7c76b6 to 03003f9 Compare October 31, 2015 19:59

saghul reviewed Dec 4, 2015
View reviewed changes

Comment thread docs/src/fs.rst Outdated

Copy link
Copy Markdown

Member

saghul Dec 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention what we use on Windows and the fact that it doesn't work on XP.

jhamhader force-pushed the realpath branch from 02e375c to 1cd5f9a Compare December 4, 2015 17:16

saghul reviewed Dec 4, 2015
View reviewed changes

Comment thread docs/src/fs.rst

Copy link
Copy Markdown

Member

saghul Dec 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be nitpicky here... can you add that it returns UV_ENOSYS?

fs: add uv_fs_realpath()

ed3cd9d

Add uv_fs_realpath() - returns the full resolved absolute path of a file or directory.

jhamhader force-pushed the realpath branch from 1cd5f9a to ed3cd9d Compare December 4, 2015 20:15

saghul pushed a commit that referenced this pull request Dec 5, 2015

fs: add uv_fs_realpath()

e76b883

Equivalent to realpath(3), returns the full resolved absolute path of a file or directory. PR-URL: #531 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul closed this Dec 5, 2015

MylesBorins mentioned this pull request Apr 22, 2016

fix: catch ELOOP isaacs/node-glob#259

Closed

bpasero mentioned this pull request Mar 16, 2017

Prevent wrongly cased file paths to enter the workbench microsoft/vscode#22740

Closed

Conversation

jhamhader commented Sep 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChALkeR Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented Sep 21, 2015

Uh oh!

jhamhader commented Sep 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented Sep 23, 2015

Uh oh!

piscisaureus commented Sep 24, 2015

Uh oh!

saghul commented Sep 24, 2015

Uh oh!

jhamhader commented Oct 8, 2015

Uh oh!

jhamhader commented Oct 18, 2015

Uh oh!

bnoordhuis commented Oct 30, 2015

Uh oh!

saghul commented Nov 4, 2015

Uh oh!

bnoordhuis commented Nov 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented Dec 4, 2015

Uh oh!

saghul commented Dec 4, 2015

Uh oh!

jhamhader commented Dec 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saghul commented Dec 4, 2015

Uh oh!

jhamhader commented Dec 4, 2015

Uh oh!

saghul commented Dec 4, 2015

Uh oh!

saghul commented Dec 5, 2015

Uh oh!

MylesBorins commented Apr 22, 2016

Uh oh!

Fishrock123 commented Oct 14, 2016

Uh oh!

saghul commented Oct 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ChALkeR Apr 22, 2016 •

edited

Loading