fix: catch ELOOP

[MylesBorins]

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]

/cc @jasnell who did all the dirty work in solving this one.

[mscdex]

s/ev_fs_realpath/uv_fs_realpath/

[isaacs]

Wow, didn't notice that uv has realpath now.

Someone should confirm that it's not using the realpath(3) UNIX function, since that's notoriously insecure. (That's why node had a realpath in JS in the first place.)

If it is, then it's better for glob to refuse to use realpath in those versions of node.

[MylesBorins]

@isaacs it is using realpath(3). I see you've joined the discussion in nodejs/node#2680 (comment)

I'll keep this thread updated with what happens in the Node project

[MylesBorins]

@isaacs if we update the version of libuv in node to include the fix for realpath(3) would you be opposed to this landing in node-glob?

[isaacs]

@thealphanerd I haven't followed the conversation in depth, but I see the some ^Lift folks got involved. If @jlamendo is happy with where node 6 lands on this, then I'm happy supporting it.

[jlamendo]

@thealphanerd From my understanding, node 6 is landing on throwing an error when PATH_MAX is undefined.

If that's correct, I'm ok with that solution. While I don't want to break anyone's build, it's better to do that than deal with a compromised machine. There seems to be a few conflicting threads on that, so I'm not sure what the actual accepted solution was.

That said, I do have a concern with the comment about encoding being in the cache. Unfortunately I didn't have time to root this out on my own today, so hopefully you know, but are there any controls in place to prevent that from happening, and are such controls possible? If it does happen, what is the impact?

[MylesBorins]

the changes to libuv landed in v6.2. Any system that does not have PATH_MAX will have an error thrown during compilation

@jlamendo can you elaborate on what you meant by encoding in the cache?

[MylesBorins]

ping @jlamendo

@isaacs this is still breaking CI for most flavors of linux for catching cycles

[isaacs]

What is the intent of this bit? The indentation is incorrect, making it look like any error that isn't a stat or ELOOP will get thrown, but in fact, the only error that'll get thrown is an ELOOP when real is falsey.

[isaacs]

For example:

if (false)
  console.log(1)
else if (false)
  if (true) console.log(2)
else
  console.log(3)

is actually

if (false)
  console.log(1)
else if (false)
  if (true)
    console.log(2)
  else
    console.log(3)

[isaacs]

Also, how can real be set if an error occurred? Does fs.realpath ever return a value if it raises an error? That seems really strange.

[MylesBorins]

added braces to fix the error. I'm going to have to dig back into the code to figure out exactly why we went with setting real... although based on your response at the end of this PR it may not be worth putting in the time.

[isaacs]

Digging into this more, it's almost impossible to fix without either (a) making a significant breaking change to glob, or (b) porting the pre-node6 fs.realpath into a standalone module and using that instead. Both options are nontrivial.

The tests are breaking because Node broke behavior. fs.realpath no longer works like it used to, and any module that exposed any kind of realpath API surface is almost certainly failing tests as well (provided that their test coverage was sufficient to detect it.)

The patch here simply ignores ELOOP errors that are raised by the fs.realpath call. That does work in the one case where a test is failing, but digging in a bit deeper, I'm not convinced that it's the correct solution in every case. In Node 6, node-glob simply cannot do what it used to be able to do. I am concerned that ignoring ELOOP will violate its contract. (Also, what about ENAMETOOLONG? It's purely lucky happenstance that the test is not raising that instead.)

It's going to take some time to fix issues like this. In the meantime, in my opinion, people should either know that they're using a not-ready-for-prime-time version of the platform (and thus, expect test failures), or avoid Node 6 until it and the ecosystem have had time to get stable. This new work lands on the laps of many part-time maintainers who weren't planning for it. I have no idea when I'll even get to it, but it's not at the top of my list. I've already spent several hours debugging new issues that Node 6 raised.

Breaking changes break stuff. This is what it looks like when a platform is unstable. It's costly.

[jlamendo]

@thealphanerd
I was referring to the statement you made - 'As long as the cache doesn't include a value called "encoding" everything will be fine.'

What is the impact if it is included in the cache, and are there any mechanisms in place to detect that before it causes problems or prevent it from being there in the first place?

It may be a minor issue, and I suspect that it is the case, but I wanted to be sure.