Windows で job_start にダブルクオート付き引数を渡すと正しくエスケープされない。 #961

Open
mattn opened this Issue Oct 2, 2016 · 12 comments

Projects

None yet

2 participants

@mattn
Member
mattn commented Oct 2, 2016 edited

#957 から派生。

job_start で第一引数が配列場合は shellescape を使ってエスケープしてますが、その際 """ にエスケープされる。ただしコマンドが期待するのは \"" なので何かしらの対処が必要。ちなみに

#include <stdio.h>

int
main(int argc, char* argv[]) {
    puts(argv[1]);
    return 0;
}

このコマンドに "" を渡す為には "\""" でないとダメ。

再現バージョン 8.0.0019 Windows10 64bit

@mattn
Member
mattn commented Oct 2, 2016

Windows では途中に \ が含まれるかどうかで " の扱いが変わるらしい。

diff --git a/src/misc2.c b/src/misc2.c
index 4d914d2..c7c3e01 100644
--- a/src/misc2.c
+++ b/src/misc2.c
@@ -1409,6 +1409,10 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
     char_u *escaped_string;
     int        l;
     int        csh_like;
+# if defined(WIN32) || defined(DOS)
+    int        escaping = 0;
+    int        has_spaces = 0;
+# endif

     /* Only csh and similar shells expand '!' within single quotes.  For sh and
      * the like we must not put a backslash before it, it will be taken
@@ -1417,14 +1421,16 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
     csh_like = csh_like_shell();

     /* First count the number of extra bytes required. */
-    length = (unsigned)STRLEN(string) + 3;  /* two quotes and a trailing NUL */
+    length = (unsigned)STRLEN(string) + 1;
     for (p = string; *p != NUL; mb_ptr_adv(p))
     {
 # if defined(WIN32) || defined(DOS)
    if (!p_ssl)
    {
-       if (*p == '"')
+       if (*p == '"' || *p == '\\')
        ++length;       /* " -> "" */
+       if (*p == ' ' || *p == '\t')
+       has_spaces = 1;
    }
    else
 # endif
@@ -1444,6 +1450,12 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
    }
     }

+# if defined(WIN32) || defined(DOS)
+    /* When has spaces on Windows, it should be quoted while string. */
+    if (has_spaces)
+   length += 2;
+# endif
+
     /* Allocate memory for the result and fill it. */
     escaped_string = alloc(length);
     if (escaped_string != NULL)
@@ -1452,9 +1464,10 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)

    /* add opening quote */
 # if defined(WIN32) || defined(DOS)
-   if (!p_ssl)
-       *d++ = '"';
-   else
+   if (!p_ssl) {
+       if (has_spaces)
+       *d++ = '"';
+   } else
 # endif
        *d++ = '\'';

@@ -1465,11 +1478,22 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
        {
        if (*p == '"')
        {
-           *d++ = '"';
+           int i;
+           for (i = 0; i < escaping; i++)
+           *d++ = '\\';
+           *d++ = '\\';
            *d++ = '"';
            ++p;
            continue;
        }
+       else
+       if (*p == '\\')
+       {
+           *d++ = *p++;
+           escaping++;
+           continue;
+       }
+       escaping = 0;
        }
        else
 # endif
@@ -1504,9 +1528,14 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)

    /* add terminating quote and finish with a NUL */
 # if defined(WIN32) || defined(DOS)
-   if (!p_ssl)
-       *d++ = '"';
-   else
+   if (!p_ssl) {
+       if (has_spaces) {
+       int i;
+       for (i = 0; i < escaping; i++)
+           *d++ = '\\';
+       *d++ = '"';
+       }
+   } else
 # endif
        *d++ = '\'';
    *d = NUL;
@mattn
Member
mattn commented Oct 2, 2016

ちなみにこの変更で

:echo system("echo " . shellescape("foo"))

foo でなく "foo" になってた問題も直してます。

@mattn
Member
mattn commented Oct 2, 2016

いっそのこと system([]) をサポートするパッチにしてしまってもいいかなーと思いますがどうでしょうか。

@mattn
Member
mattn commented Oct 2, 2016

あ、いや無しだ。Windows の cmd.exe で空白付きの echo をサポート仕切れないので system([]) のサポートは無しです。exec([]) ならありだったけど。

@mattn mattn self-assigned this Oct 2, 2016
@k-takata
Member
k-takata commented Oct 2, 2016

エスケープの仕様は何をもって正しいとするのでしょうか。vim自身が使っている CommandLineToArgvW の動作は :help gui-w32-cmdargs, :help win32-quotes に書いてありますが、MSVCRTの動作とも微妙に違いますし、MinGWとも違うかもしれません。

@mattn
Member
mattn commented Oct 2, 2016

今の僕の認識だと、シェル(cmd)を通すかどうかで動作が変わるはずで、今回の様にシェルを通さない job_start([]) の場合には shellescape は使えないという認識です。上記のパッチは shellescape に当ててしまいましたが、フラグか何かを設けて job_start([]) 向けのエスケープと通常の shellescape を分けるべきだと思います。それを証拠に

#include <stdio.h>

int
main(int argc, char* argv[]) {
    puts(argv[1]);
    return 0;
}

にシングルクオートを1つ渡すのに

:echo system("foo " . shellescape('"'))

とすると現状のパッチだとE484が発生します。ただし現状の vim が作るエスケープ("""")だと job_start には使えません。

@mattn
Member
mattn commented Oct 2, 2016

job_start([]) の場合だけ今回の変更を行う様にしました。

diff --git a/src/channel.c b/src/channel.c
index 13fb653..427a213 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -4764,7 +4764,7 @@ job_start(typval_T *argvars)
         * double quote and backslashes. */
        p_ssl = 0;
 # endif
-       s = vim_strsave_shellescape(s, FALSE, TRUE);
+       s = vim_strsave_shellescape(s, FALSE, TRUE, TRUE);
 # ifdef WIN32
        p_ssl = old_ssl;
 # endif
diff --git a/src/eval.c b/src/eval.c
index 3b5abe9..45c92cd 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -9862,7 +9862,7 @@ repeat:
    c = (*fnamep)[*fnamelen];
    if (c != NUL)
        (*fnamep)[*fnamelen] = NUL;
-   p = vim_strsave_shellescape(*fnamep, FALSE, FALSE);
+   p = vim_strsave_shellescape(*fnamep, FALSE, FALSE, FALSE);
    if (c != NUL)
        (*fnamep)[*fnamelen] = c;
    if (p == NULL)
diff --git a/src/evalfunc.c b/src/evalfunc.c
index 84eaa8c..4201301 100644
--- a/src/evalfunc.c
+++ b/src/evalfunc.c
@@ -10355,7 +10355,7 @@ f_sha256(typval_T *argvars, typval_T *rettv)
 f_shellescape(typval_T *argvars, typval_T *rettv)
 {
     rettv->vval.v_string = vim_strsave_shellescape(
-       get_tv_string(&argvars[0]), non_zero_arg(&argvars[1]), TRUE);
+   get_tv_string(&argvars[0]), non_zero_arg(&argvars[1]), TRUE, FALSE);
     rettv->v_type = VAR_STRING;
 }

diff --git a/src/misc2.c b/src/misc2.c
index 4d914d2..4debcee 100644
--- a/src/misc2.c
+++ b/src/misc2.c
@@ -1401,7 +1401,8 @@ csh_like_shell(void)
  * Returns the result in allocated memory, NULL if we have run out.
  */
     char_u *
-vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
+vim_strsave_shellescape(char_u *string,
+   int do_special, int do_newline, int do_list)
 {
     unsigned   length;
     char_u *p;
@@ -1409,6 +1410,10 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
     char_u *escaped_string;
     int        l;
     int        csh_like;
+# if defined(WIN32) || defined(DOS)
+    int        escaping = 0;
+    int        has_spaces = 0;
+# endif

     /* Only csh and similar shells expand '!' within single quotes.  For sh and
      * the like we must not put a backslash before it, it will be taken
@@ -1417,14 +1422,16 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
     csh_like = csh_like_shell();

     /* First count the number of extra bytes required. */
-    length = (unsigned)STRLEN(string) + 3;  /* two quotes and a trailing NUL */
+    length = (unsigned)STRLEN(string) + 1;
     for (p = string; *p != NUL; mb_ptr_adv(p))
     {
 # if defined(WIN32) || defined(DOS)
    if (!p_ssl)
    {
-       if (*p == '"')
+       if (*p == '"' || (do_list && *p == '\\'))
        ++length;       /* " -> "" */
+       if (*p == ' ' || *p == '\t')
+       has_spaces = 1;
    }
    else
 # endif
@@ -1444,6 +1451,12 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
    }
     }

+# if defined(WIN32) || defined(DOS)
+    /* When has spaces on Windows, it should be quoted while string. */
+    if (has_spaces)
+   length += 2;
+# endif
+
     /* Allocate memory for the result and fill it. */
     escaped_string = alloc(length);
     if (escaped_string != NULL)
@@ -1453,7 +1466,10 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
    /* add opening quote */
 # if defined(WIN32) || defined(DOS)
    if (!p_ssl)
-       *d++ = '"';
+   {
+       if (has_spaces || !do_list)
+       *d++ = '"';
+   }
    else
 # endif
        *d++ = '\'';
@@ -1465,11 +1481,27 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
        {
        if (*p == '"')
        {
-           *d++ = '"';
+           if (do_list)
+           {
+           int i;
+           for (i = 0; i < escaping; i++)
+               *d++ = '\\';
+           *d++ = '\\';
+           }
+           else
+           *d++ = '"';
            *d++ = '"';
            ++p;
            continue;
        }
+       else
+       if (do_list && *p == '\\')
+       {
+           *d++ = *p++;
+           escaping++;
+           continue;
+       }
+       escaping = 0;
        }
        else
 # endif
@@ -1505,7 +1537,18 @@ vim_strsave_shellescape(char_u *string, int do_special, int do_newline)
    /* add terminating quote and finish with a NUL */
 # if defined(WIN32) || defined(DOS)
    if (!p_ssl)
-       *d++ = '"';
+   {
+       if (has_spaces || !do_list)
+       {
+       if (do_list)
+       {
+           int i;
+           for (i = 0; i < escaping; i++)
+           *d++ = '\\';
+       }
+       *d++ = '"';
+       }
+   }
    else
 # endif
        *d++ = '\'';
diff --git a/src/normal.c b/src/normal.c
index 8302ffb..18e3359 100644
--- a/src/normal.c
+++ b/src/normal.c
@@ -5647,7 +5647,7 @@ nv_ident(cmdarg_T *cap)
     {
    /* Escape the argument properly for a shell command */
    ptr = vim_strnsave(ptr, n);
-   p = vim_strsave_shellescape(ptr, TRUE, TRUE);
+   p = vim_strsave_shellescape(ptr, TRUE, TRUE, FALSE);
    vim_free(ptr);
    if (p == NULL)
    {
diff --git a/src/proto/misc2.pro b/src/proto/misc2.pro
index d18ae20..0683dea 100644
--- a/src/proto/misc2.pro
+++ b/src/proto/misc2.pro
@@ -35,7 +35,7 @@ char_u *vim_strnsave(char_u *string, int len);
 char_u *vim_strsave_escaped(char_u *string, char_u *esc_chars);
 char_u *vim_strsave_escaped_ext(char_u *string, char_u *esc_chars, int cc, int bsl);
 int csh_like_shell(void);
-char_u *vim_strsave_shellescape(char_u *string, int do_special, int do_newline);
+char_u *vim_strsave_shellescape(char_u *string, int do_special, int do_newline, int do_list);
 char_u *vim_strsave_up(char_u *string);
 char_u *vim_strnsave_up(char_u *string, int len);
 void vim_strup(char_u *p);
@mattn
Member
mattn commented Oct 2, 2016

なんか job_start([]) 向けに新しい関数用意した方がいい気がしてきたw ただ今後シェルを介さないsystem()などの事を考えるとshellescapeと同居してもいいって気もする。

@mattn
Member
mattn commented Oct 3, 2016

ひとまずこのまま行きます。あとでテスト書きます。python を起動して print をコールバックする様な物にする予定です。

@mattn
Member
mattn commented Oct 3, 2016

良く考えたら shell 介さないのに p_ssl 依存なのは変すぎるな。関数分けよう。

@mattn
Member
mattn commented Oct 3, 2016

この修正を確認するテストを書こうにも #957 が治らないと確認できないというツラさ。終了コードで確認するのを途中まで書いてみた物のコレジャナイ感。

diff --git a/src/testdir/test_channel.vim b/src/testdir/test_channel.vim
index eb75a0b..e7c7c8e 100644
--- a/src/testdir/test_channel.vim
+++ b/src/testdir/test_channel.vim
@@ -1503,5 +1503,24 @@ func Test_close_lambda()
   call s:run_server('Ch_test_close_lambda')
 endfunc

+func Test_job_start_windows()
+  if !has('job') || !has('win32')
+    return
+  endif
+  let s:test_job_start_msg = ''
+  let g:job = job_start(['vim', '-u', 'NONE', '-c', 'echo "hello"', '-c', 'quitall!'], {'callback': {ch,msg->execute('let s:test_job_start_msg=msg')}})
+  try
+    call WaitFor('job_status(g:job) == "dead"')
+    call assert_equal('"hello"', s:test_job_start_msg)
+  finally
+    call Stop_g_job()
+    unlet s:test_job_start_msg
+  endtry
+endfunc
+
 " Uncomment this to see what happens, output is in src/testdir/channellog.
 " call ch_logfile('channellog', 'w')

いっそ #957 に混ぜてもらおうかと検討中。

@k-takata
Member
k-takata commented Oct 4, 2016

最初の python の例ですが、python の公式バイナリだとどちらも同じように動きますね。
MinGWとCommandLineToArgvW()が同じ動作?

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