lib: Make escapeShellArg more robust

Quoting various characters that the shell *may* interpret specially is a
very fragile thing to do.

I've used something more robust all over the place in various Nix
expression I've written just because I didn't trust escapeShellArg.

Here is a proof of concept showing that I was indeed right in
distrusting escapeShellArg:

with import <nixpkgs> {};

let
  payload = runCommand "payload" {} ''
    # \x00 is not allowed for Nix strings, so let's begin at 1
    for i in $(seq 1 255); do
      echo -en "\\x$(printf %02x $i)"
    done > "$out"
  '';

  escapers = with lib; {
    current = escapeShellArg;
    better = arg: let
      backslashEscapes = stringToCharacters "\"\\ ';$`()|<>\r\t*[]&!~#";
      search = backslashEscapes ++ [ "\n" ];
      replace = map (c: "\\${c}") backslashEscapes ++ [ "'\n'" ];
    in replaceStrings search replace (toString arg);
    best = arg: "'${replaceStrings ["'"] ["'\\''"] (toString arg)}'";
  };

  testWith = escaper: let
    escaped = escaper (builtins.readFile payload);
  in runCommand "test" {} ''
    if ! r="$(bash -c ${escapers.best "echo -nE ${escaped}"} 2> /dev/null)"
    then
      echo bash eval error > "$out"
      exit 0
    fi
    if echo -n "$r" | cmp -s "${payload}"; then
      echo success > "$out"
    else
      echo failed > "$out"
    fi
  '';

in runCommand "results" {} ''
  echo "Test results:"
  ${lib.concatStrings (lib.mapAttrsToList (name: impl: ''
    echo "  ${name}: $(< "${testWith impl}")"
  '') escapers)}
  exit 1
''

The resulting output is the following:

Test results:
  best: success
  better: success
  current: bash eval error

I did the "better" implementation just to illustrate that the method of
quoting only "harmful" characters results in madness in terms of
implementation and performance.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Cc: @edolstra, @zimbatm

1 files changed, 7 insertions, 8 deletions

diff --git a/lib/strings.nix b/lib/strings.nix
index 04376a5f2fbe..5e5f7b378667 100644
--- a/lib/strings.nix
+++ b/lib/strings.nix
@@ -203,20 +203,19 @@ rec {
   */
   escape = list: replaceChars list (map (c: "\\${c}") list);
 
-  /* Escape all characters that have special meaning in the Bourne shell.
+  /* Quote string to be used safely within the Bourne shell.
 
      Example:
-       escapeShellArg "so([<>])me"
-       => "so\\(\\[\\<\\>\\]\\)me"
+       escapeShellArg "esc'ape\nme"
+       => "'esc'\\''ape\nme'"
   */
-  escapeShellArg = arg:
-    lib.escape (stringToCharacters "\\ ';$`()|<>\t*[]") (toString arg);
+  escapeShellArg = arg: "'${replaceStrings ["'"] ["'\\''"] (toString arg)}'";
 
-  /* Escape all arguments to be passed to the Bourne shell.
+  /* Quote all arguments to be safely passed to the Bourne shell.
 
      Example:
-       escapeShellArgs ["one" "two three"]
-       => "one two\\ three"
+       escapeShellArgs ["one" "two three" "four'five"]
+       => "'one' 'two three' 'four'\\''five'"
   */
   escapeShellArgs = concatMapStringsSep " " escapeShellArg;