Repository navigation
DebugAdapterExecutable does not support passing shell: true so .bat-based debug adapters are broken in current Insiders #224184
Description
In current Insiders (and presumably the upcoming release), spawning a .bat file on Windows without shell: true fails due to a breaking change in NodeJS:
https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
It is important to note that there has been a breaking change for Windows users who utilize
child_process.spawnandchild_process.spawnSync. Node.js will now error withEINVALif a.bator.cmdfile is passed tochild_process.spawnandchild_process.spawnSyncwithout theshelloption set.
It appears that there's no way to signal this to DebugAdapterExecutable and therefore any .bat-based debug adapters launched this way fail in current Insiders (and will presumably fail in stable soon).
Could shell: bool? be added to DebugAdapterExecutableOptions? I don't know enough to contribute this, but this seems quite important.
Activity
DanTup commentedon Jul 29, 2024
DanTup commentedon Jul 29, 2024
Add the ability to use shell:true for DebugAdapterExecutable
DanTup commentedon Jul 29, 2024
I have a fix in a PR at #224204
Although because I sneakily have a space in the path to my DA, I also noticed that enabling
shell: truealso changes behaviour, and the path to the executable will need quoting/escaping. I've called this out on the doc forshellbecause it's something extension authors will need to consider when switching to use this.Set DA to use shell:true on Windows for flutter
DanTup commentedon Jul 29, 2024
roblourens I noticed you assigned this to the August milestone - does this mean that the next stable will ship without a fix for this? It breaks debug adapters that use
.batlike Flutter, which will affect a considerable number of users.roblourens commentedon Jul 29, 2024
Well, I can't take the PR for the next Stable because it adds extension API and we can't add that two days before we release. I'll take a look at this.
Maybe you could wrap your
.batin a node script where you can callspawnyourself, and vscode can run the node script? You just have to connect the stdin and stdout pipes.connor4312 commentedon Jul 30, 2024
Please see gitkraken/vscode-gitlens#3387 for guidance -- that was the issue template we automated posting on on extensions that contained
batfiles. I don't think there's any 'fix' for this we can apply on the VS Code side without reverting the CVE patch.This is not really related to DAP. DAP does not know anything about the runtime that debug adapters are executed in.
DanTup commentedon Jul 30, 2024
roblourens
The Flutter extension has almost 9m installs and this issue breaks starting debug sessions for all of them on Windows. Surely if a critical issue like this is discovered right before a release, some solution should be looked at? The change could be made without an API change by having VS Code just check for
.baton the end of the DA executable, but I'm not sure if that's a better fix.This might work, but I suspect there are edge cases (like handling termination of additional processes) that IMO make this a riskier fix to ship the day before a release than adding
shell: true.Connor Peet (@connor4312)
Dart-Code never got this template (it doesn't contain
batfiles, the Flutter debug adapter lives in the Flutter SDK). If I did, I could've sent the PR weeks ago :-(I think you have misunderstood the issue. I am familiar with the breaking change - this is a process being spawned by VS Code, not the extension:
vscode/src/vs/workbench/contrib/debug/node/debugAdapter.ts
Line 231 in 59b9ae6
All my extension code has always used
shell: truewhere necessary, but this is code where we provide the executable and args to VS Code and it spawns the process - it does not use (and gives us no ability to pass)shell: true. Please see my linked PR which fixes the issue.1 remaining item
Spawn Flutter debug adapter directly with dart.exe
Spawn Flutter debug adapter directly with dart.exe
roblourens commentedon Jul 30, 2024
I just discussed this with Connor, and we agree that this is different than the other issues. We can likely check for a DebugAdapterExecutable command that ends in
.batand set the shell flag ourselves (and escape spaces). We can do that on vscode's end without new API. And maybe can get that in before the next stable release. I will try that out today.DanTup commentedon Jul 30, 2024
Thank you! I was preparing a workaround to ship in Dart-Code, however it's far from ideal (it hard-codes assumptions about what the shell script does that may not hold across all Flutter SDK versions) and it would only work for users that update to the latest extension (whereas a fix in VS Code as you describe would prevent anyone from breaking).
I may still ship the workaround in a pre-release today to get some additional testing so if you cannot fix this I at least have a fallback, but I'd appreciate if you could keep me posted with your testing and when it might hit insiders, because I need to decide whether to include the workaround in the stable extension release (which is likely to be tomorrow or Thursday).
Tnx! :)
Spawn Flutter debug adapter directly with dart.exe
Spawn Flutter debug adapter directly with dart.exe
roblourens commentedon Jul 30, 2024
/closedBy 3bb1578