This repository has been archived by the owner. It is now read-only.

UNC paths on search_path for uv_spawn #748

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@paulgoldsmith
Contributor

paulgoldsmith commented Mar 17, 2013

Support UNC paths on the PATH env when searching for an executable to spawn a process

@paulgoldsmith

This comment has been minimized.

Show comment
Hide comment
@paulgoldsmith

paulgoldsmith Mar 17, 2013

Contributor

While this solves the UNC path problem (and hopefully does not break existing cases), there is another corner case this does not account for. With OpenAFS Windows File System Redirector if a file is referenced using an @sys variable in the path the GetFileAttributeW call returns an attribute of FILE_ATTRIBUTE_REPARSE_POINT, so even though it is a file capable of being executed the success criteria for search_path_join_test to return true is not met.

  if (attrs != INVALID_FILE_ATTRIBUTES &&
     !(attrs & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT))) {
    return result;
  }

This post goes in to a little more detail of the redirector implementation with regards to reparse points http://cygwin.com/ml/cygwin-developers/2013-02/msg00161.html

I would be interested in why the decision to make this ignore reparse points was originally made. It is worth noting that if I remove the reparse point mask from the expression the spawn is able to successfully execute a file with an @sys in the path. The workaround of course is to specify the fully resolved file path instead of using an @sys variable.

I guess the question remains how it would handle a case where the base name is a directory with an extension specified or .exe/.com.

Contributor

paulgoldsmith commented Mar 17, 2013

While this solves the UNC path problem (and hopefully does not break existing cases), there is another corner case this does not account for. With OpenAFS Windows File System Redirector if a file is referenced using an @sys variable in the path the GetFileAttributeW call returns an attribute of FILE_ATTRIBUTE_REPARSE_POINT, so even though it is a file capable of being executed the success criteria for search_path_join_test to return true is not met.

  if (attrs != INVALID_FILE_ATTRIBUTES &&
     !(attrs & (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT))) {
    return result;
  }

This post goes in to a little more detail of the redirector implementation with regards to reparse points http://cygwin.com/ml/cygwin-developers/2013-02/msg00161.html

I would be interested in why the decision to make this ignore reparse points was originally made. It is worth noting that if I remove the reparse point mask from the expression the spawn is able to successfully execute a file with an @sys in the path. The workaround of course is to specify the fully resolved file path instead of using an @sys variable.

I guess the question remains how it would handle a case where the base name is a directory with an extension specified or .exe/.com.

@paulgoldsmith

This comment has been minimized.

Show comment
Hide comment
@paulgoldsmith

paulgoldsmith Jun 13, 2013

Contributor

Here is a little (well a lot!) more detail on how OpenAFS makes use of the reparse point attribute.

http://blog.secure-endpoints.com/2013/03/symbolic-links-on-windows.html

Contributor

paulgoldsmith commented Jun 13, 2013

Here is a little (well a lot!) more detail on how OpenAFS makes use of the reparse point attribute.

http://blog.secure-endpoints.com/2013/03/symbolic-links-on-windows.html

- if (dir_len >= 1 && (dir[0] == L'/' || dir[0] == L'\\')) {
+ if (dir_len > 2 && dir[0] == L'\\' && dir[1] == L'\\') {
+ /* It's a UNC path so ignore cwd */
+ cwd_len = 0;

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 18, 2013

Contributor

Use two spaces for indentation, not tabs.

@bnoordhuis

bnoordhuis Jun 18, 2013

Contributor

Use two spaces for indentation, not tabs.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Jun 18, 2013

@paulgoldsmith

Yes, this should be fixed. UNC paths can appear in two places (namely the to-be-spawned filename, and the PATH env var) and it took me a while to see how this fixes both cases - but it seems that it does.

A couple of comments before I can land it:

  • Treat both // and \\ as UNC path indicators. This is what cmd.exe does as well (and we're trying to follow that).
C:\Users\Bert Belder>set path=//./c:\windows\system32

C:\Users\Bert Belder>cmd
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\Bert Belder>
  • Replace the comment you're removing with something that says, "yes, we are dealing with UNC paths in both the path and the filename. This is a deviation from what cmd.exe does - it does not let you start a program by specifying an UNC path on the command line - but this is really a pointless restriction."
  • Use two-space indents and not tabs.
  • Please sign the node.js CLA.

@paulgoldsmith

Yes, this should be fixed. UNC paths can appear in two places (namely the to-be-spawned filename, and the PATH env var) and it took me a while to see how this fixes both cases - but it seems that it does.

A couple of comments before I can land it:

  • Treat both // and \\ as UNC path indicators. This is what cmd.exe does as well (and we're trying to follow that).
C:\Users\Bert Belder>set path=//./c:\windows\system32

C:\Users\Bert Belder>cmd
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\Bert Belder>
  • Replace the comment you're removing with something that says, "yes, we are dealing with UNC paths in both the path and the filename. This is a deviation from what cmd.exe does - it does not let you start a program by specifying an UNC path on the command line - but this is really a pointless restriction."
  • Use two-space indents and not tabs.
  • Please sign the node.js CLA.
@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Jun 18, 2013

@paulgoldsmith

As for the OpenAFS redirector issue.

  • The reason we're filtering out directories early is because cmd.exe does that too. See

    libuv/src/win/process.c

    Lines 314 to 317 in b1d390e

    * - When cmd.exe cannot read a directory, it will just skip it and go on
    * searching. However, unlike posix-y systems, it will happily try to run a
    * file that is not readable/executable; if the spawn fails it will not
    * continue searching.
  • Filtering out reparse points is indeed incorrect. I remember that when I wrote that code I was under the impression that GetFileAttributes would "follow" a reparse point just like CreateFile. That assumption I just figured out was wrong. It's somewhat surprising that you're the first one to report this, because this also breaks any attempt to spawn a .exe file that's symlinked. I will fix this.
D:\>mklink test.exe c:\windows\system32\cmd.exe
symbolic link created for test.exe <<===>> c:\windows\system32\cmd.exe

D:\>test
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

D:\>exit

D:\>node -pe "void require('child_process').spawn('test.exe')
undefined

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: spawn ENOENT
    at errnoException (child_process.js:980:11)
    at Process.ChildProcess._handle.onexit (child_process.js:771:34)

D:\>node -v
v0.10.8

@paulgoldsmith

As for the OpenAFS redirector issue.

  • The reason we're filtering out directories early is because cmd.exe does that too. See

    libuv/src/win/process.c

    Lines 314 to 317 in b1d390e

    * - When cmd.exe cannot read a directory, it will just skip it and go on
    * searching. However, unlike posix-y systems, it will happily try to run a
    * file that is not readable/executable; if the spawn fails it will not
    * continue searching.
  • Filtering out reparse points is indeed incorrect. I remember that when I wrote that code I was under the impression that GetFileAttributes would "follow" a reparse point just like CreateFile. That assumption I just figured out was wrong. It's somewhat surprising that you're the first one to report this, because this also breaks any attempt to spawn a .exe file that's symlinked. I will fix this.
D:\>mklink test.exe c:\windows\system32\cmd.exe
symbolic link created for test.exe <<===>> c:\windows\system32\cmd.exe

D:\>test
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

D:\>exit

D:\>node -pe "void require('child_process').spawn('test.exe')
undefined

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: spawn ENOENT
    at errnoException (child_process.js:980:11)
    at Process.ChildProcess._handle.onexit (child_process.js:771:34)

D:\>node -v
v0.10.8

piscisaureus added a commit that referenced this pull request Jun 18, 2013

windows: uv_spawn shouldn't reject reparse points
This fixes an issue where uv_spawn would not try to run a reparse point,
and continue to scan the PATH instead. Effectively, it was impossible to
spawn a symlinked binary. This commit fixes that.

Also see #748

piscisaureus added a commit that referenced this pull request Jun 18, 2013

windows: uv_spawn shouldn't reject reparse points
This fixes an issue where uv_spawn would not try to run a reparse point,
and continue to scan the PATH instead. Effectively, it was impossible to
spawn a symlinked binary. This commit fixes that.

Also see #748

This is a backport of 495d1a0 to the v0.8 branch.
@paulgoldsmith

This comment has been minimized.

Show comment
Hide comment
@paulgoldsmith

paulgoldsmith Jun 19, 2013

Contributor

Thanks Ben and Bert. I'll investigate signing the CLA tomorrow.

Contributor

paulgoldsmith commented Jun 19, 2013

Thanks Ben and Bert. I'll investigate signing the CLA tomorrow.

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Jun 19, 2013

@paulgoldsmith If the CLA turns out to be a problem, I can do the changes myself. Let me know what works.

@paulgoldsmith If the CLA turns out to be a problem, I can do the changes myself. Let me know what works.

@paulgoldsmith

This comment has been minimized.

Show comment
Hide comment
@paulgoldsmith

paulgoldsmith Jun 26, 2013

Contributor

So the signing of the CLA is taking longer than I had hoped. Go ahead and do the changes, please. Cheers.

Contributor

paulgoldsmith commented Jun 26, 2013

So the signing of the CLA is taking longer than I had hoped. Go ahead and do the changes, please. Cheers.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Jun 27, 2014

Contributor

It has been a while... :-S Do we still need to land this @piscisaureus? We no longer have a CLA, mind rebasing @paulgoldsmith?

Contributor

saghul commented Jun 27, 2014

It has been a while... :-S Do we still need to land this @piscisaureus? We no longer have a CLA, mind rebasing @paulgoldsmith?

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Jul 19, 2014

@saghul yes, I think is can be landed now that the cla requirement is lifted (but I had a couple of comments in review)

@saghul yes, I think is can be landed now that the cla requirement is lifted (but I had a couple of comments in review)

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Aug 7, 2014

Contributor

@piscisaureus I rebased and addressed the comments here: saghul/libuv@ef74328, does it LGTY?

Contributor

saghul commented Aug 7, 2014

@piscisaureus I rebased and addressed the comments here: saghul/libuv@ef74328, does it LGTY?

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Aug 10, 2014

Contributor

That took a while... landed in ac879ed, thanks! :-)

Contributor

saghul commented Aug 10, 2014

That took a while... landed in ac879ed, thanks! :-)

@saghul saghul closed this Aug 10, 2014

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