ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
184 stars 31 forks source link

Completion fails after `$'`…`\'`…`'` #462

Closed McDutchie closed 2 years ago

McDutchie commented 2 years ago

Reproducer: On an interactive shell with emacs or vi on, type a command with a $'' quoted string that contains a backslash-escaped single quote, like:

$ true $'foo\'bar' ▁

Then begin to type the name of a file present in the current working directory and press tab. Nothing happens as completion fails to work.

Hypothesis: the completion code does not recognise $'' strings. Instead, it parses them as '' strings in which there are no backslash escapes, so it considers the last ' to open a second quoted string which is not terminated.

McDutchie commented 2 years ago

Support for $'' will need to be implemented in the find_begin() function: https://github.com/ksh93/ksh/blob/a8f5c3cf6d7ac975f5a4becebbdaa282645bc528/src/cmd/ksh93/edit/completion.c#L106-L214

McDutchie commented 2 years ago

Here is a patch. @hyenias and @JohnoKing, could you try to break it?

diff --git a/src/cmd/ksh93/edit/completion.c b/src/cmd/ksh93/edit/completion.c
index bd5fdfc34..b949f6fa4 100644
--- a/src/cmd/ksh93/edit/completion.c
+++ b/src/cmd/ksh93/edit/completion.c
@@ -105,12 +105,17 @@ static char *overlaid(register char *str,register const char *newstr,int nocase)

 /*
  * returns pointer to beginning of expansion and sets type of expansion
+ *
+ * Detects three quoting styles:
+ * 1. '...'    inquote=='\'', dollarquote==0; no special characters
+ * 2. $'...'   inquote=='\'', dollarquote==1; skips \.
+ * 3. "..."    inquote=='"',  dollarquote==0; skips \., $..., ${...}, $(...), `...`
  */
 static char *find_begin(char outbuff[], char *last, int endchar, int *type)
 {
    register char   *cp=outbuff, *bp, *xp;
-   register int    c,inquote = 0, inassign=0;
-   int     mode=*type;
+   char        inquote = 0, dollarquote = 0, inassign = 0;
+   int     mode=*type, c;
    bp = outbuff;
    *type = 0;
    mbinit();
@@ -127,10 +132,10 @@ static char *find_begin(char outbuff[], char *last, int endchar, int *type)
                break;
            }
            if(inquote==c)
-               inquote = 0;
+               inquote = dollarquote = 0;
            break;
            case '\\':
-           if(inquote != '\'')
+           if(inquote != '\'' || dollarquote)
                mbchar(cp);
            break;
            case '$':
@@ -173,6 +178,8 @@ static char *find_begin(char outbuff[], char *last, int endchar, int *type)
                if(*(cp=xp)!=')')
                    bp = xp;
            }
+           else if(c=='\'' && !inquote)
+               dollarquote = 1;
            break;
            case '`':
            if(inquote=='\'')
JohnoKing commented 2 years ago

I found some odd behavior, although it appears to be a separate bug (your patch did not introduce this problem).

# Completing a file name after a quote deletes the quote
$ '/tm<Tab>
$ /tmp        # The quote is gone

# This example shows that the dollar sign isn't deleted; only the quote is
$ $'/etc/hosts<Tab>
$ $/etc/hosts

# The example below results in ksh attempting to execute a directory
$ '`/bi<Tab>
$ '`/bi/home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh[1]: /bin/: cannot execute [Is a directory]
McDutchie commented 2 years ago

I found some odd behavior, although it appears to be a separate bug (your patch did not introduce this problem).

# Completing a file name after a quote deletes the quote
$ '/tm<Tab>
$ /tmp        # The quote is gone

I'm pretty sure that one is as expected; the completion code re-quotes quoted words completely using its own algorithm (see fmtx()) that uses backslash escaping only. Which leaves no quoting at all for that string.

# This example shows that the dollar sign isn't deleted; only the quote is
$ $'/etc/hosts<Tab>
$ $/etc/hosts

Hmm. Looks like my patch to find_begin() is not yet complete; it considers the ' to be the beginning of the quoted stirng.

# The example below results in ksh attempting to execute a directory
$ '`/bi<Tab>
$ '`/bi/home/johno/GitRepos/KornShell/ksh/arch/linux.i386-64/bin/ksh[1]: /bin/: cannot execute [Is a directory]

Ugh. Apparently, #268 was not completely fixed.

McDutchie commented 2 years ago

New version of the patch that fixes this problem:

# This example shows that the dollar sign isn't deleted; only the quote is
$ $'/etc/hosts<Tab>
$ $/etc/hosts

To fix this, find_begin() returns a special type D for a $'' string which is then recognised by ed_expand() (note we cannot use $ as that is already used for variable expansions).

Still to fix: spurious execution of command substitutions as in the previous comment.

diff --git a/src/cmd/ksh93/edit/completion.c b/src/cmd/ksh93/edit/completion.c
index bd5fdfc34..e29bdc031 100644
--- a/src/cmd/ksh93/edit/completion.c
+++ b/src/cmd/ksh93/edit/completion.c
@@ -105,12 +105,17 @@ static char *overlaid(register char *str,register const char *newstr,int nocase)

 /*
  * returns pointer to beginning of expansion and sets type of expansion
+ *
+ * Detects three quoting styles:
+ * 1. '...'    inquote=='\'', dollarquote==0; no special characters
+ * 2. $'...'   inquote=='\'', dollarquote==1; skips \.
+ * 3. "..."    inquote=='"',  dollarquote==0; skips \., $..., ${...}, $(...), `...`
  */
 static char *find_begin(char outbuff[], char *last, int endchar, int *type)
 {
    register char   *cp=outbuff, *bp, *xp;
-   register int    c,inquote = 0, inassign=0;
-   int     mode=*type;
+   char        inquote = 0, dollarquote = 0, inassign = 0;
+   int     mode=*type, c;
    bp = outbuff;
    *type = 0;
    mbinit();
@@ -127,10 +132,10 @@ static char *find_begin(char outbuff[], char *last, int endchar, int *type)
                break;
            }
            if(inquote==c)
-               inquote = 0;
+               inquote = dollarquote = 0;
            break;
            case '\\':
-           if(inquote != '\'')
+           if(inquote != '\'' || dollarquote)
                mbchar(cp);
            break;
            case '$':
@@ -173,6 +178,8 @@ static char *find_begin(char outbuff[], char *last, int endchar, int *type)
                if(*(cp=xp)!=')')
                    bp = xp;
            }
+           else if(c=='\'' && !inquote)
+               dollarquote = 1;
            break;
            case '`':
            if(inquote=='\'')
@@ -209,7 +216,10 @@ static char *find_begin(char outbuff[], char *last, int endchar, int *type)
        }
    }
    if(inquote && *bp==inquote)
-       *type = *bp++;
+   {
+       *type = dollarquote ? 'D' : *bp;
+       bp++;
+   }
    return(bp);
 }

@@ -348,7 +358,11 @@ int ed_expand(Edit_t *ep, char outbuff[],int *cur,int *eol,int mode, int count)
            com = sh_argbuild(&narg,comptr,0);
            /* special handling for leading quotes */
            if(begin>outbuff && (begin[-1]=='"' || begin[-1]=='\''))
-           begin--;
+           {
+               begin--;
+               if(var=='D')        /* $'...' */
+                   begin--;    /* also remove initial dollar */
+           }
        }
        sh_offstate(SH_COMPLETE);
                 /* allow a search to be aborted */