Skip to content

Escapes cmd.exe Special Characters#5030

Open
myurasov-nv wants to merge 2 commits intoisaac-sim:developfrom
myurasov-nv:my-nvbugs-5978469
Open

Escapes cmd.exe Special Characters#5030
myurasov-nv wants to merge 2 commits intoisaac-sim:developfrom
myurasov-nv:my-nvbugs-5978469

Conversation

@myurasov-nv
Copy link
Collaborator

When subprocess.run() receives a list whose first element is a .bat file, Python invokes cmd.exe /c "..." under the hood. subprocess.list2cmdline executed down the line does not escape cmd.exe special characters (<, >, |, &, ^), so they are interpreted as shell operators, which breaks commands that utilize IsacSim's python.bat as interpreter ("C:\Users\Misha\IsaacLab\_isaac_sim\python.bat -m pip install setuptools<82.0.0" etc).

run_command() now detects .bat/.cmd executables on Windows and converts the command list to a pre-quoted string, bypassing list2cmdline.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • [NA] I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Mar 16, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR fixes a Windows-specific bug where cmd.exe metacharacters (<, >, |, &, ^) in subprocess arguments were being interpreted as shell operators when invoking .bat/.cmd files via subprocess.run(). The fix adds a _escape_for_cmd_exe() helper that detects .bat/.cmd executables and converts the command list to a pre-quoted string, bypassing Python's subprocess.list2cmdline which doesn't handle these characters.

  • The real-world trigger is run_command(pip_cmd + ["install", "setuptools<82.0.0"]) where pip_cmd starts with python.bat — the < was interpreted as a shell redirect
  • The approach of returning a string instead of a list is the correct way to bypass list2cmdline in Python's subprocess module
  • Minor hardening opportunity: arguments with embedded double quotes are not escaped before wrapping in quotes (unlikely with current callers but worth noting for robustness)

Confidence Score: 4/5

  • This PR is safe to merge — it fixes a real Windows bug with a well-scoped, platform-guarded change that only activates on Windows with .bat/.cmd executables.
  • The fix is correct and well-targeted. It properly handles the known limitation of Python's list2cmdline not escaping cmd.exe metacharacters. The only minor concern is the lack of escaping for embedded double quotes in arguments, but this is a defensive hardening concern rather than a real-world bug with current callers. Deducting 1 point for the missing embedded-quote handling and the absence of tests.
  • No files require special attention — the single changed file is straightforward and well-contained.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/cli/utils.py Adds _escape_for_cmd_exe() helper to properly quote cmd.exe metacharacters when invoking .bat/.cmd files on Windows. Correctly bypasses list2cmdline by returning a pre-quoted string. Minor concern: embedded double quotes in arguments are not escaped.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["run_command(cmd, ...)"] --> B{"cmd is list/tuple<br>AND is_windows()?"}
    B -- No --> F["subprocess.run(cmd, ...)"]
    B -- Yes --> C["_escape_for_cmd_exe(cmd)"]
    C --> D{"cmd[0] ends with<br>.bat or .cmd?"}
    D -- No --> E["Return list(cmd) unchanged"]
    D -- Yes --> G["Quote args with metacharacters<br>or whitespace"]
    G --> H["Return joined string<br>(bypasses list2cmdline)"]
    E --> F
    H --> F
Loading

Last reviewed commit: 711410c

Comment on lines +163 to +166
for arg in cmd:
s = str(arg)
if any(c in s for c in _CMD_METACHARACTERS) or " " in s or "\t" in s:
parts.append(f'"{s}"')
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguments with embedded double quotes will produce malformed quoting

If an argument already contains a " character (e.g., a path like C:\some"dir\file), wrapping it in f'"{s}"' produces "C:\some"dir\file" which cmd.exe will parse incorrectly. While this is unlikely with current callers (pip package specs, flags, paths), it would be safer to escape any existing double quotes before wrapping:

Suggested change
for arg in cmd:
s = str(arg)
if any(c in s for c in _CMD_METACHARACTERS) or " " in s or "\t" in s:
parts.append(f'"{s}"')
if any(c in s for c in _CMD_METACHARACTERS) or " " in s or "\t" in s:
parts.append(f'"{s.replace(chr(34), chr(34)+chr(34))}"')

In cmd.exe, "" inside a double-quoted string represents a literal ". This is a minor defensive hardening — current callers don't pass arguments with embedded quotes, but future callers might.

Copy link
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants