Skip to content
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

Prevent corruption of disassembler listing command for paths w/ spaces #253

Merged
merged 1 commit into from
Oct 1, 2022
Merged

Prevent corruption of disassembler listing command for paths w/ spaces #253

merged 1 commit into from
Oct 1, 2022

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Oct 1, 2022

This platform uses a build hook to save a disassembler listing to the build folder when compiling sketches.

Due to the use of a redirection operator in the command, it was necessary to execute it via the Windows command interpreter by passing it as an argument to cmd /c.

Although paths with spaces can be used without difficulty in pattern generated commands executed directly by the Arduino framework, things are more complicated when it comes to executing commands indirectly via cmd /c.

The cmd /c command has an odd handling of quotes:

https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/cmd#remarks

Under certain conditions, the first and last quotation mark of the command passed via the argument to /c are stripped.

These conditions occur for the command generated via the platform's recipe.hooks.objcopy.postobjcopy.1.pattern.windows property when the both the path to avr-objdump and the temporary build path contain a space.

The removal of the essential quotes from the start and end of the command executed by cmd /c causes an error. for example, if the avr-objdump path was:

C:\Users\username with spaces\AppData\Local\\Arduino15\packages\arduino\tools\avr-gcc\7.3.0-atmel3.6.1-arduino7\bin\avr-objdump

The command will fail with the error:

'C:\Users\username' is not recognized as an internal or external command, operable program or batch file.

One of the required conditions for the unwanted quote stripping is that the first character of the argument to cmd /c is a double quote. So this unwanted behavior can be avoided by ensuring the first character is not a double quote. That is accomplished here by adding a dummy command (adds a blank line to the output, but the most suitable "no operation" candidate I found) that does not require quoting at the start of the argument.

Demo of bug

$ arduino-cli version
arduino-cli.exe  Version: 0.27.1 Commit: a900cfb2 Date: 2022-09-18T15:57:51Z

$ export ARDUINO_DIRECTORIES_DATA="/tmp/arduino-cli-directories/has space/data" # Cause avr-objdump to be installed under a path with spaces

$ export ARDUINO_BOARD_MANAGER_ADDITIONAL_URLS="https://mcudude.github.io/MightyCore/package_MCUdude_MightyCore_index.json"

$ arduino-cli core update-index
Downloading index: package_index.tar.bz2 downloaded
Downloading index: package_MCUdude_MightyCore_index.json downloaded

$ arduino-cli core install MightyCore:avr

[...]

$ export ARDUINO_DIRECTORIES_USER="/tmp/arduino-cli-directories/user"

$ git clone --depth 1 https://github.com/MCUdude/MightyCore "${ARDUINO_DIRECTORIES_USER}/hardware/MightyCore-dev"

[...]

$ arduino-cli sketch new "/tmp/FooSketch"

Sketch created in: C:\Users\per\AppData\Local\Temp\FooSketch

$ arduino-cli compile --fqbn MightyCore-dev:avr:1284 --verbose --build-path "/tmp/arduino-cli-directories/has space/build" "/tmp/FooSketch"

[...]

cmd /C "C:\\Users\\per\\AppData\\Local\\Temp\\arduino-cli-directories\\has space\\data\\packages\\arduino\\tools\\avr-gcc\\7.3.0-atmel3.6.1-arduino7/bin/avr-objdump" --disassemble --source --line-numbers --demangle --section=.text "C:\\Users\\per\\AppData\\Local\\Temp\\arduino-cli-directories\\has space\\build/FooSketch.ino.elf" > "C:\\Users\\per\\AppData\\Local\\Temp\\arduino-cli-directories\\has space\\build/FooSketch.ino_atmega1284p_16000000L.lst"
'C:\Users\per\AppData\Local\Temp\arduino-cli-directories\has' is not recognized as an internal or external command,
operable program or batch file.

[...]

(Arduino CLI used only because it makes it easy to customize the paths to contain spaces)

Additional information

Alternatives considered

It might seem that a less hacky approach would be to simply wrap the entire argument to cmd /c in an extra layer of sacrificial quotes, leaving the essential quotes intact in the command that is finally executed. While that does indeed work when running the command directly from the command line, it does not work when used in a pattern in the Arduino platform configuration framework. The problem is that the string generated from the platform pattern can not be executed as is. It must be split into arguments. The list of arguments space separated, with quoting where an argument contains a space.

These arguments may contain quote characters, which must be escaped. This escaping is where the simple approach of extra quotes becomes unusable. The os/exec Go package used to handle the escaping does that in a way that is compatible with directly executing a command, but is not compatible with the legacy escaping system used by cmd:

https://pkg.go.dev/os/exec#Command

On Windows, processes receive the whole command line as a single string and do their own parsing. Command combines and quotes Args into a command line string with an algorithm compatible with applications using CommandLineToArgvW (which is the most common way). Notable exceptions are msiexec.exe and cmd.exe (and thus, all batch files), which have a different unquoting algorithm. In these or other similar cases, you can do the quoting yourself and provide the full command line in SysProcAttr.CmdLine, leaving Args empty.

Scope of bug

It seems this problem of the command having paths with spaces did not occur in Arduino IDE 1.x due to it using a Windows "8.3" format for the default build path, which does not contain spaces. Arduino CLI (and thus Arduino IDE 2.x) uses the full path format instead. However, the bug could also occur in Arduino IDE 1.x if the user sets a custom build.path with spaces.

recipe.hooks.savehex.presavehex.2.pattern.windows also uses cmd /C. However, this command is not subject to the quote stripping because it does not ever have a double quote as the first character of the argument.

Related

Originally reported by @cattledogGH

Previous discussion at MCUdude/MegaCoreX#159

This platform uses a build hook to save a disassembler listing to the build folder when compiling sketches.

Due to the use of a redirection operator in the command, it was necessary to execute it via the Windows command
interpreter by passing it as an argument to `cmd /c`.

Although paths with spaces can be used without difficulty in pattern generated commands executed directly by the Arduino
framework, things are more complicated when it comes to executing commands indirectly via `cmd /c`.

The `cmd /c` command has an odd handling of quotes:

https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/cmd#remarks

Under certain conditions, the first and last quotation mark of the command passed via the argument to `/c` are stripped.

These conditions occur for the command generated via the platform's `recipe.hooks.objcopy.postobjcopy.1.pattern.windows`
property when the both the path to `avr-objdump` and the temporary build path contain a space.

The removal of the essential quotes from the start and end of the command executed by `cmd /c` causes an error. for
example, if the `avr-objdump` path was:

```
C:\Users\username with spaces\AppData\Local\\Arduino15\packages\arduino\tools\avr-gcc\7.3.0-atmel3.6.1-arduino7\bin\avr-objdump
```

The command will fail with the error:

'C:\Users\username' is not recognized as an internal or external command, operable program or batch file.

One of the required conditions for the unwanted quote stripping is that the first character of the argument to `cmd /c`
is a double quote. So this unwanted behavior can be avoided by ensuring the first character is not a double quote. That
is accomplished here by adding a dummy command that does not require quoting at the start of the argument.

It might seem that a less hacky approach would be to simply wrap the entire argument to `cmd /c` in an extra layer of
sacrificial quotes, leaving the essential quotes intact in the command that is finally executed. While that does indeed
work when running the command directly from the command line, it does not work when used in a pattern in the Arduino
platform configuration framework. The problem is that the string generated from the platform pattern can not be executed
as is. It must be split into arguments. The list of arguments space separated, with quoting where an argument contains a
space.

These arguments may contain quote characters, which must be escaped. This escaping is where the simple approach of extra
quotes becomes unusable. The `os/exec` Go package used to handle the escaping does that in a way that is compatible with
directly executing a command, but is not compatible with the legacy escaping system used by cmd.

It seems this problem of the command having paths with spaces did not occur in Arduino IDE 1.x due to it using a Windows
"8.3" format instead. Arduino CLI (and thus Arduino IDE 2.x) use the full path format instead.

It seems this problem of the command having paths with spaces did not occur in Arduino IDE 1.x due to it using a Windows
"8.3" format for the default build path, which meant the path in the command did not contain spaces. Arduino CLI (and
thus Arduino IDE 2.x) uses the full path format instead. However, the bug could also occur in Arduino IDE 1.x if the
user sets a custom build.path with spaces.

`recipe.hooks.savehex.presavehex.2.pattern.windows` also uses `cmd /C`. However, this command is not subject to the
quote stripping because it does not ever have a double quote as the first character of the argument.
@MCUdude
Copy link
Owner

MCUdude commented Oct 1, 2022

Thanks for the PR!

@MCUdude MCUdude merged commit b360c00 into MCUdude:master Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants