New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process, win: fix shell spawn with AutoRun #8063

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@bzoz
Contributor

bzoz commented Aug 11, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process, win

Description of change

Under Windows system can be configured to execute a specific command each time a shell is spawned. Under some conditions this breaks the way node handles shell scripts under windows. This commit adds /d switch to spawn and spawnSync which disables this AutoRun functionality.

Fixes: nodejs/node-v0.x-archive#25458

child_process, win: fix shell spawn with AutoRun
Under Windows system can be configured to execute a specific command each time
a shell is spawned. Under some conditions this breaks the way node handles
shell scripts under windows. This commit adds /d switch to spawn and spawnSync
which disables this AutoRun functionality.

Fixes: nodejs/node-v0.x-archive#25458
@bzoz

This comment has been minimized.

Contributor

bzoz commented Aug 11, 2016

This is a breaking change, if users depend on the AutoRun feature. It is also the only way to fix issues like nodejs/node-v0.x-archive#25458. In that specific configuration, node child_process tests fail.

cc @nodejs/platform-windows @nodejs/ctc

@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Aug 17, 2016

LGTM

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

@nodejs/ctc According to https://github.com/nodejs/node/blob/a1d3a8db50fcbb3fb0639cb1bd8077d2c290a21b/COLLABORATOR_GUIDE.md#accepting-modifications , this has to be reviewed by several members of the CTC. Should this be added to the CTC agenda in this case?

This is a breaking change because some users might be relying on AutoRun commands to run before their shell scripts. Such users should be able to add the commands otherwise (e. g. running a batch file with the AutoRun commands and the previously run command). I am not aware of any situation where AutoRun is essential for the shell to function. On the other hand, the motivation for this is that some users are stuck with a AutoRun configuration that they cannot change, as in the referenced issue. Furthermore, module writers have no way to know what is in their users AutoRun, so this might help there as well.

@Trott

This comment has been minimized.

Member

Trott commented Aug 17, 2016

@joaocgreis Probably no need to put it on the CTC agenda unless one of the following happens:

  • it becomes too contentious as to whether the change should land at all
  • it is difficult or impossible to get two CTC members to provide an LGTM for it

I'd say leave it off the agenda this week, but if it's stuck on Friday or Monday, add the ctc-agenda label then.

CTC folks I'm guessing would be the likely ones to take an interest: @bnoordhuis @cjihrig @rvagg

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Aug 17, 2016

The code changes themselves LGTM, but I'll defer to the Windows experts. Just out of curiosity, what do other languages do in this situation?

@bzoz

This comment has been minimized.

Contributor

bzoz commented Aug 18, 2016

Python for example uses cmd.exe /c. People encounter issues with spaces in filename and with quoting parameters (stackoverflow question).

@rvagg

This comment has been minimized.

Member

rvagg commented Aug 31, 2016

It'd be nice to have an understanding of what people use \Software\Microsoft\Command Processor\AutoRun for. Googling doesn't really make me feel educated on the topic. Some details on the kinds of breakage this is intended to fix would be nice too, or is nodejs/node-v0.x-archive#25458 all we have to go on?

Intuitively it would seem to me that if you have something set to run each time cmd.exe is run then it should even run when Node invokes it. If you want to get around this behaviour then you could always spawn cmd.exe /d instead of using exec.

@bzoz the python problem is unrelated to this PR though isn't it? That's because of the lack of /s which we apparently already use?

@rvagg rvagg removed the ctc-agenda label Aug 31, 2016

@rvagg

This comment has been minimized.

Member

rvagg commented Aug 31, 2016

@joaocgreis & @bzoz we had a brief discussion @ CTC meeting about this today but with a lack of Windows experts we were unable to make any progress. @joshgav is going to see if he can find an authoritative source in Microsoft to offer an opinion. We have an LGTM from @joaocgreis so we might need him to champion this to the CTC because the default status (sorry, thanks to me repeating my suggestion above and @joshgav making a similar statement) is not enthusiastic.

@rvagg

This comment has been minimized.

Member

rvagg commented Aug 31, 2016

Oh, and this can go back on ctc-agenda at any time if a collaborator wants to do that.

@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Sep 1, 2016

The autorun feature seems to be relatively little used, I'm not aware of any official recommendation to use it or any situation where it is the best solution for a problem that would interfere here. By Googling, it seems the main use for this feature is to run cd to start the Command Prompt in a specific directory. Other uses include printing warnings on elevated command prompts and loading user specific aliases. (for reference, here's a blog post with informative comments)

This is only intended to fix nodejs/node-v0.x-archive#25458 (node apparently ignoring the cwd option). There's another possible scenario though: if a npm package makes use of exec, a user might accidentally break it if there is a autorun setting.

Currently, exec will use the autorun setting, but a user can opt-out by running spawn with cmd directly. It seems to me that this would make more sense inverted, as this PR suggests, because the code the user is trying to run might come from a npm package. Autorun might be useful for a user running on his environment but it might be a problem for packages, and currently there is no easy way to opt-out.

Python and Rust don't seem to have a way to run batch files without running cmd directly. .NET has System.Diagnostics.Process.Start that respects autorun, but can only execute a file with an array of arguments, not any expression (like tasklist | grep node). It actually checks if the file is a batch file, and starts it with full path, hence eliminating the problem when autorun is a cd command. I am not aware of another language with a general function like exec, so we have no references here.

I believe this PR is a improvement for node, but I can't be sure about all node users out there, so I completely respect if the CTC believes the risk is too great for this fix.

@addaleax

This comment has been minimized.

Member

addaleax commented Sep 1, 2016

@joshgav

This comment has been minimized.

Member

joshgav commented Sep 6, 2016

@joaocgreis

code the user is trying to run might come from a npm package

This is a good point IMHO. For these users the straightforward fix would be this PR. Asking them to somehow edit up their dependencies won't scale.

Working on getting an opinion from the command-line team.

@joshgav

This comment has been minimized.

Member

joshgav commented Sep 7, 2016

The console team in Windows agreed with adding /d by default, in particular cause of Joao's point:

There's another possible scenario though: if a npm package makes use of exec, a user might accidentally break it if there is a autorun setting.

So LGTM!

/cc @miniksa

@joshgav joshgav added this to the 7.0.0 milestone Sep 7, 2016

@rvagg

This comment has been minimized.

Member

rvagg commented Sep 8, 2016

this lgtm given that upstream advice and @joaocgreis' comments

@joaocgreis do you want to handle merging this one?

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 8, 2016

@rvagg @joshgav @addaleax ... if we're going to get this into v7 then it would need to land before the morning of Monday Sept 12th. I'm going to be cutting the v7.x branch that morning to start the beta process.

@addaleax

This comment has been minimized.

Member

addaleax commented Sep 8, 2016

I think there’s consensus here, so I’m going to land this tomorrow unless somebody else does or objects.

@addaleax

This comment has been minimized.

Member

addaleax commented Sep 9, 2016

Landed in b90f3da, with the commit message re-wrapped at 72 columns.

@addaleax addaleax closed this Sep 9, 2016

addaleax added a commit that referenced this pull request Sep 9, 2016

child_process, win: fix shell spawn with AutoRun
Under Windows system can be configured to execute a specific command
each time a shell is spawned. Under some conditions this breaks the
way node handles shell scripts under windows.
This commit adds /d switch to spawn and spawnSync which disables this
AutoRun functionality.

Fixes: nodejs/node-v0.x-archive#25458
PR-URL: #8063
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Sep 12, 2016

I was out, thanks @addaleax !

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

@joaocgreis joaocgreis referenced this pull request Jul 31, 2018

Closed

child-process: support any shells on Windows #21943

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment