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

Unable to pass & in an argument to a shell script started with Process.start #59604

Closed
DanTup opened this issue Nov 25, 2024 · 6 comments
Closed
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DanTup
Copy link
Collaborator

DanTup commented Nov 25, 2024

I'm trying to fix a bug where we mishandle some characters like & in user-provided arguments because of having to spawn flutter.bat through a shell. There is a little info on Process.start() about this:

NOTE: On Windows, if executable is a batch file ('.bat' or '.cmd'), it may be launched by the operating system in a system shell regardless of the value of runInShell. This could result in arguments being parsed according to shell rules. For example:

  // Will launch notepad.
  Process.start('test.bat', ['test&notepad.exe']);
}

The issue I'm having is that I can't find any way to escape a & that comes through correctly.

Below is a script to reproduce the issue. It creates a temp folder with a space in the name, and then writes a simple .bat file that forwards all args to a Dart script (similar to what flutter.bat does). The Dart script it writes just prints all the arguments out to stdout. The test reads stdout and compares what's printed to what it sent as arguments to ensure they match.

If I remove the & from testCharacters, the test passes. If I add the & then it fails because the last argument is truncated, and it tried to execute after:

'after\""' is not recognized as an internal or external command,
operable program or batch file.
Expected: ['beforeaafter', 'beforebafter', 'beforecafter', 'before&after']
  Actual: ['beforeaafter', 'beforebafter', 'beforecafter', '"before']

The _escapeAndQuoteArg function needs to escape & in some way, but I've tried many combinations (including backslashes, the ^ character and combinations of quoting/not quoting the args) (I'm assuming https://ss64.com/nt/syntax-esc.html is a reasonable source), but none of them work. Based on @derekxu16 comment at #50076 (comment) it's not clear to me if Dart is also trying to do some of this escaping.

I'm not sure if this is a bug, or I'm doing it wrong. I'm hoping someone that understands the code in createProcess may be able to verify one way or the other.

import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as path;
import 'package:test/test.dart';

const testCharacters = r'abc&'; // r' "&:\<>|%=+-_@~#<>?/';

final testArgs = [
  for (var char in testCharacters.split('')) 'before${char}after',
];

void main() {
  test('test escaping', () async {
    var tempDir =
        Directory.systemTemp.createTempSync('flutter dap args escape test');
    print('Writing scripts to $tempDir');

    // Write a shell script to simulate the Flutter .bat file and a Dart script
    // that just prints the arguments sent to it one-per-line.
    var tempShellScript = File(path.join(tempDir.path, 'fake_flutter.bat'));
    var tempDartScript = File(path.join(tempDir.path, 'print_args.dart'));
    var command = [
      '"${Platform.resolvedExecutable}"',
      '"${tempDartScript.path}"',
      "%*",
    ];
    tempShellScript.writeAsStringSync('@echo off\n${command.join(" ")}');
    tempDartScript.writeAsStringSync(r'''
void main(List<String> args) {
  print(args.join('\n'));
}
''');

    var executable = tempShellScript.path;
    var args = testArgs.map(_escapeAndQuoteArg).toList();

    print('''
Executing:
  executable: $executable
  args: ${args.join(' ')}
  runInShell: true
''');
    var proc = await Process.start(
      '"$executable"',
      args,
    );

    var stdoutFuture = proc.stdout.transform(utf8.decoder).toList();
    var stderrFuture = proc.stderr.transform(utf8.decoder).toList();
    await proc.exitCode;

    var stdout = (await stdoutFuture).join().trim();
    var stderr = (await stderrFuture).join().trim();

    if (stderr.isNotEmpty) {
      print(stderr);
    }

    var actual = stdout
        .split('\n')
        .map((l) => l.trim())
        .where((l) => l.isNotEmpty)
        .toList();

    expect(actual, testArgs);
  });
}

String _escapeAndQuoteArg(String input) {
  // What is the correct thing to do here to escape &?
  // return input.replaceAll('&', '^&');
  return input;
}
@DanTup
Copy link
Collaborator Author

DanTup commented Nov 25, 2024

@brianquinlan I'm told that you might be knowledgeable in this area :-)

@dart-github-bot
Copy link
Collaborator

Summary: User cannot escape & in arguments passed to a Windows batch script via Process.start. The shell interprets & before Process.start can handle it, leading to argument truncation.

@dart-github-bot dart-github-bot added area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Nov 25, 2024
@mraleph
Copy link
Member

mraleph commented Nov 26, 2024

You can make this test pass by doing ^^^&. Explanation: you need to escape & once on the way to the first batch file and then you need to escape it once more because of how %* works... So if you know that you are going through flutter.bat/dart.bat and you know that they are going to forward arguments using %* then you can be prepared for that, otherwise I think you are out of luck.

This is also a reason why flutter.bat/dart.bat approach is kinda sorta broken on Windows.

See also this SO question for gnarly details.

@DanTup
Copy link
Collaborator Author

DanTup commented Nov 26, 2024

and then you need to escape it once more because of how %* works

Thank you! I actually wondered if it was related to %* but hadn't thought about it enough to realise this would be the fix.

So there's no bug here, and we'll probably have to double-escape these for now then (generally we know we are going through Flutter... it might break if users configure a customTool but I think there's not much we can do if it ends with .bat but doesn't do the same).

It would be nice to come up with a better solution (like compiling a non-.bat entry point for Flutter), but that's unlikely to be simple.

(FYI @bkonyi)

@DanTup DanTup closed this as completed Nov 26, 2024
@DanTup
Copy link
Collaborator Author

DanTup commented Nov 26, 2024

@mraleph I'm not sure if I may have still found a bug.. interested in your opinion.

Firstly, I decided that maybe it's better to escape inside flutter.bat where %* is used, rather than double-escape here? Presumably we already have bugs from the command line if it's not re-escaping the args correctly? (I will verify this if I thinkw e should go ahead with this).

So I have an updated test script below which writes the escaping into the shell script (in reality this would go into flutter.bat):

SET "args=%*"
SETLOCAL EnableDelayedExpansion
SET "args=!args:^=^^!"
SET "args=!args:&=^&!"
SET "args=!args:|=^|!"
SET "args=!args:<=^<!"
SET "args=!args:>=^>!"
SET "args=!args:\=^\!"

"D:\Tools\Dart\Stable\bin\dart.exe" "C:\Users\danny\AppData\Local\Temp\flutter dap args escape test67432968\print_args.dart" %args%

It seems to work for every character currently in the escape set (the test passes).. however, if I add a double quote to the test characters, it fails because the output contains ^" where only " is expected - however none of this code (as far as I can tell) ever escapes double quotes, so I don't know where this is coming from (could it be Dart?).

import 'dart:convert';
import 'dart:io';
import 'package:path/path.dart' as path;
import 'package:test/test.dart';

final charactersToEscape = r'^&|<>\';
// Adding double quote " into the set of the test characters breaks, because
// we get back ^" even though we don't escape a double quote anywhere.
const testCharacters = r'abc ^&|<>\:%=+-_@~#<>?/';

final testArgs = [
  for (var char in testCharacters.split('')) 'before${char}after',
];

void main() {
  test('test escaping', () async {
    var tempDir =
        Directory.systemTemp.createTempSync('flutter dap args escape test');
    print('Writing scripts to $tempDir');

    // Write a shell script to simulate the Flutter .bat file and a Dart script
    // that just prints the arguments sent to it one-per-line.
    var tempShellScript = File(path.join(tempDir.path, 'fake_flutter.bat'));
    var tempDartScript = File(path.join(tempDir.path, 'print_args.dart'));
    var shellScriptContents = '''
@echo off

SET "args=%*"
SETLOCAL EnableDelayedExpansion
${charactersToEscape.split('').map((c) => 'SET "args=!args:$c=^$c!"').join('\n')}

"${Platform.resolvedExecutable}" "${tempDartScript.path}" %args%
''';
    tempShellScript.writeAsStringSync(shellScriptContents);
    tempDartScript.writeAsStringSync(r'''
void main(List<String> args) {
  print(args.join('\n'));
}
''');

    var executable = tempShellScript.path;
    var args = testArgs.map(_escapeAndQuoteArg).toList();

    print('''
Executing:
  executable: $executable
  args: ${args.join(' ')}
  runInShell: true
''');
    var proc = await Process.start(
      '"$executable"',
      args,
    );

    var stdoutFuture = proc.stdout.transform(utf8.decoder).toList();
    var stderrFuture = proc.stderr.transform(utf8.decoder).toList();
    await proc.exitCode;

    var stdout = (await stdoutFuture).join().trim();
    var stderr = (await stderrFuture).join().trim();

    if (stderr.isNotEmpty) {
      print(stderr);
    }

    var actual = stdout
        .split('\n')
        .map((l) => l.trim())
        .where((l) => l.isNotEmpty)
        .toList();

    expect(actual, testArgs);
  });
}

String _escapeAndQuoteArg(String input) {
  final escapedChars = RegExp.escape(charactersToEscape);
  return input.replaceAllMapped(
      new RegExp('[$escapedChars]'), (m) => '^${m.group(0)}');
}

@mraleph
Copy link
Member

mraleph commented Nov 26, 2024

SETLOCAL EnableDelayedExpansion

FWIW some variations involving delayed expansion were tried before but it caused its own bunch of problems, see flutter/flutter#84270

I am also not sure you can just really escape things reliably like that.

It seems to work for every character currently in the escape set (the test passes).. however, if I add a double quote to the test characters, it fails because the output contains ^" where only " is expected - however none of this code (as far as I can tell) ever escapes double quotes, so I don't know where this is coming from (could it be Dart?).

Yeah, there is some escaping in Process implementation on Windows which was put there in ancient times ff7d45d. This logic might easily be borked FWIW, I am not entirely sure why is it there specifically for quotes.

As a result of that escaping you get \\" on entry to the batch file, and that somehow morphs into ^" when you do %args% at the last line. I am actually not sure how that happens. I have tried printing ECHO %args% but that yields "before^\\"after" and not before^"after... Rules for these substitutions seem to be seriously out of this world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

3 participants