Skip to content

Conversation

@mjrist
Copy link
Contributor

@mjrist mjrist commented Jul 30, 2025

Summary
Fixes #13791 by making the remote attach picker respect the pipeTransport.quoteArgs configuration.

@mjrist mjrist requested a review from a team as a code owner July 30, 2025 18:47
@github-project-automation github-project-automation bot moved this to Pull Request in cpptools Jul 30, 2025
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone know why it's specified as pipeTransport.quoteArgs.exceptions? Is true/false valid for quoteArgs?

Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be pipeTransport.quoteArgs?.exceptions?

UPDATE: Actually, it seems like the references to the "exceptions" object in quoteArgs should be removed? I don't understand why that was used.

At MIEngine I see it without "exceptions" so I think that should be removed: https://github.com/microsoft/MIEngine/blob/main/src/MIDebugPackage/OpenFolderSchema.json#L253

Although we could do that change as a follow up fix ourselves.

sean-mcmanus
sean-mcmanus previously approved these changes Jul 30, 2025
@mjrist
Copy link
Contributor Author

mjrist commented Jul 30, 2025

@microsoft-github-policy-service agree company="SEL"

@Colengms
Copy link
Contributor

Colengms commented Jul 30, 2025

It's worth noting that simply quoting or not quoting arguments is not a proper way to deal with whether arguments are expected to or expected not to potentially contain shell quoting/escaping. Whether or not arguments are expected to potentially contain or not to contain shell quoting/escaping, are mutually exclusive. (There's no auto-detect potential, as the same content could be interpreted differently depending on that assumption.)

https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

https://daviddeley.com/autohotkey/parameters/parameters.htm

Adding quotes may deal with use of spaces, but not much else. In general, there should be a distinction made between args vs. command-line fragments. An 'arg' (alone) is a code-level concept and does not imply anything related to shell quoting/escaping. This would seem to be particularly true when referring to an array of args, which I would propose is explicitly not intended to represent use of those arguments on a shell command line. Command lines (and fragments) are specifically related to invoking a command line using a shell (at least on Linux and macOS. On Windows, the situation is a bit murkier, as the "shell" decoding is handled by a runtime library in the target process, which can depend on how the process was built). A 'command line (or partial/fragment)' is something that would be specifically related to passing something through a shell, and therefore shell escaping is expected to potentially be present already, if needed.

As long as these are clearly distinguished in the input, there are separate spawn/launch APIs (or options to those APIs) to identify whether the arguments should receive shell processing or not.

$0.02

@sean-mcmanus sean-mcmanus merged commit 83ba416 into microsoft:main Jul 31, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from Pull Request to Done in cpptools Jul 31, 2025
@mjrist mjrist deleted the dev/make-process-picker-respect-quoteArgs branch July 31, 2025 00:27
sean-mcmanus added a commit that referenced this pull request Aug 7, 2025
* Fix the description of debugServerPath (#13778)

This mentions the non-existent miDebugServerAddress, but the
correct name is actually miDebuggerServerAddress.

* Enable string length encoding fix in cpptools (#13769)

* Try to fix the Windows builds. (#13788)

* Update IntelliSense loc strings. (#13793)

* Makes remote attach picker respect the pipeTransport.quoteArgs config… (#13794)

* Makes remote attach picker respect the pipeTransport.quoteArgs configuration
* fixes linter error - don't compare boolean value to a boolean

* Remove "exceptions" from quoteArgs. (#13796)

* Fix loc for the miDebuggerServerAddress change. (#13797)

* Update changelog for 1.27.0 (2nd time). (#13795)

* Update changelog for 1.27.0 (2nd time).

* Update form-data. (#13800)

* Update form-data.

* Revert didOpen changes in favor of adding encoding to didChangeVisibleTextEditors (#13802)

* fixing formatting (#13810)

* Enable CG trigger on insiders branch

* Handle .txx/tpp headers. (#13811)

* Handle .txx headers.

* Add tpp too.

* fix #13818 (#13824)

* Bump tmp from 0.2.3 to 0.2.4 in /Extension (#13825)

Bumps [tmp](https://github.com/raszi/node-tmp) from 0.2.3 to 0.2.4.
- [Changelog](https://github.com/raszi/node-tmp/blob/master/CHANGELOG.md)
- [Commits](raszi/node-tmp@v0.2.3...v0.2.4)

---
updated-dependencies:
- dependency-name: tmp
  dependency-version: 0.2.4
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sean McManus <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Joshua Goins <[email protected]>
Co-authored-by: Colen Garoutte-Carson <[email protected]>
Co-authored-by: Matt <[email protected]>
Co-authored-by: Bob Brown <[email protected]>
Co-authored-by: Luca <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Remote process picking appears to be broken.

3 participants