Spawn .bat/.cmd DebugAdapterExecutables with shell=true #224320
Conversation
| // https://github.com/microsoft/vscode/issues/224184 | ||
| spawnOptions.shell = true; | ||
| spawnCommand = `"${command}"`; | ||
| spawnArgs = args.map(a => `"${a}"`); |
There was a problem hiding this comment.
If any args contain " do those need escaping?
(mine don't, and hopefully others don't, but who knows?)
There was a problem hiding this comment.
IIRC things like "<" can cause issues too (see https://ss64.com/nt/syntax-esc.html). In Dart-Code I have this escape function:
I don't know if it's reliable (particularly if you changed COMSPEC to PoSh, but hopefully nobody is doing that 😄) or how likely it is in the debug adapter executable (it came up for me because test names can be passed as args to some processes and that's a shared escape function).
There was a problem hiding this comment.
Are you already escaping any args that go to the DebugAdapterExecutable? Or is it a fixed set of args?
There was a problem hiding this comment.
No, not escaping anything for DebugAdapterExecutable because it's not usually required (the function above is used in the places where we are using shell:true to spawn our own processes).
Or is it a fixed set of args?
Yep, for the place that will come through DebugAdapterExecutable our args are fixed (well, they're dynamic, but from a small set which is basically ["debug_adapter", "--test", "--no-dds"]). There are a few small exceptions (where you might pass a full path), but they're generally just for me (or other contributors to the debug adapter) that are running from source.
(TL;DR; simple escaping such as only putting quotes around them is definitely good enough for me, and the comments above were just general comments about being strict in case others are doing weirder things)
There was a problem hiding this comment.
This level of escaping seems to be fine, including for characters like >, although cmd.exe escaping is obviously extremely weird.
Did you test that this works for your scenarios @DanTup?
|
(btw, typo in title/ |
* Spawn .bat DAs with shell=true * Also escape args * And .cmd * escape argument properly * Don't escape \
Spawn .bat/.cmd DebugAdapterExecutables with shell=true (#224320) * Spawn .bat DAs with shell=true * Also escape args * And .cmd * escape argument properly * Don't escape \
|
I just checked out 3bb1578 (branch Thank you for this, I really appreciate it - it will have saved us a lot of pain and duplicate GH issues 😄
|
|
Thanks! |
…4320) * Spawn .bat DAs with shell=true * Also escape args * And .cmd * escape argument properly * Don't escape \
Fix #224184
I'm unsure about escaping the args, because I think that in the previous node version, they would have had to be escaped already, but I'll check