search

microsoft / vscode-makefile-tools

MAKE integration in Visual Studio Code
Other
185 stars 55 forks source link

Fix issues with compiler switch parsing and do command substitution for commands marked as safe #409

Open bobbrow opened 1 year ago

bobbrow commented 1 year ago

We no longer attempt to emulate shell command parsing. Instead we delegate that task to the shell itself. Some interesting cases present themselves on *nix platforms where backticks are interpreted as inline commands to be run. We can't trust all commands by default as that could be a security risk, so a new setting is also added to manage which commands are safe to run when we do the command line parsing.

My first iteration of a fix added a new parameter to util.spawnChildProcess which motivated me to consolidate the arguments into an object. That change wasn't strictly necessary after a few iterations where the command execution was moved to a different part of the code. However, I think the change adds value to the maintainability of the code, so I kept it.

Some formatting changes also happened to take place as part of this, but they are mostly whitespace related. To simplify the review, please hide whitespace changes.

A future change may build on this and change ToolInvocation.arguments into an array of strings. There are a few functions that still look through the string for specific switches using regular expressions and those regex's could potentially be removed if the arguments are already split into an array.

gcampbell-msft commented 1 year ago

Overall, I don't see any issues with this PR. However, I'll default to @andreeis for when we want to wrap this up and merge, and then include in a release.

andreeis commented 1 year ago

Overall, I don't see any issues with this PR. However, I'll default to @andreeis for when we want to wrap this up and merge, and then include in a release.

I have several changes applied on top of this and in the end I stopped when found some aspects that required more testing. I'll update this PR later with what I found and I believe we should prioritize for 0.8 (didn't create the project yet).