NetBSD Problem Report #54510

From www@netbsd.org  Fri Aug 30 12:37:17 2019
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 40B137A180
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 30 Aug 2019 12:37:17 +0000 (UTC)
Message-Id: <20190830123716.7D31A7A18D@mollari.NetBSD.org>
Date: Fri, 30 Aug 2019 12:37:16 +0000 (UTC)
From: rokuyama.rk@gmail.com
Reply-To: rokuyama.rk@gmail.com
To: gnats-bugs@NetBSD.org
Subject: libedit: filename completion broken in quote/double quaote
X-Send-Pr-Version: www-1.0

>Number:         54510
>Category:       lib
>Synopsis:       libedit: filename completion broken in quote/double quaote
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    abhinav
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 30 12:40:00 +0000 2019
>Closed-Date:    Sat Dec 31 06:19:54 +0000 2022
>Last-Modified:  Sat Dec 31 06:19:54 +0000 2022
>Originator:     Rin Okuyama
>Release:        9.99.10
>Organization:
Department of Physics, Meiji University
>Environment:
NetBSD latipes 9.99.10 NetBSD 9.99.10 (AMD64) #0: Tue Aug 27 23:08:36 JST 2019  rin@latipes:/build/src/sys/arch/amd64/compile/AMD64 amd64
>Description:
Filename completion is broken in quote/double quote since
lib/libedit/filecomplete.c rev 1.53:

http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/filecomplete.c#rev1.53

For example, with /bin/sh:

	$ pwd
	/
	$ ls '<TAB><TAB><TAB>...
	-->
	$ ls '
	(nothing happens)

Also:

	$ ls '/<TAB>
	-->
	$ ls '/'
	(the shortest candidate is selected?)

With filecomplete.c rev 1.52, both work fine:

	$ ls '<TAB><TAB>
	.cshrc      boot.cfg    etc/        libdata/    netbsd.old  stand/
	...
	$ ls '/<TAB><TAB>
	/.cshrc      /boot.cfg    /etc/        /libdata/    /netbsd.old  /stand/
	...
>How-To-Repeat:
Described above.
>Fix:
N/A, but as a workaround, reverting filecomplete rev 1.53:

http://www.netbsd.org/~rin/libedit_revert_filecomplete_1.53.patch

fixes the problems for me.

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: lib-bug-people->abhinav
Responsible-Changed-By: maya@NetBSD.org
Responsible-Changed-When: Thu, 05 Sep 2019 16:31:41 +0000
Responsible-Changed-Why:
Notify committer of patch that it is problematic.


From: "Abhinav Upadhyay" <abhinav@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54510 CVS commit: src/lib/libedit
Date: Sun, 8 Sep 2019 05:50:59 +0000

 Module Name:	src
 Committed By:	abhinav
 Date:		Sun Sep  8 05:50:58 UTC 2019

 Modified Files:
 	src/lib/libedit: filecomplete.c
 	src/lib/libedit/TEST: test_filecompletion.c

 Log Message:
 PR lib/54510: Fix file completion inside quotes which broke in rev 1.53

 While there also fix handling character appending in the file completions when
 inside quotes. For example when inside a quote, if the completion is a directory then
 append a '/' but don't close the quote. On the other hand when inside a quote if the
 completion is a file name and it is the only match then we can close the quote.


 To generate a diff of this commit:
 cvs rdiff -u -r1.57 -r1.58 src/lib/libedit/filecomplete.c
 cvs rdiff -u -r1.4 -r1.5 src/lib/libedit/TEST/test_filecompletion.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: abhinav@NetBSD.org
State-Changed-When: Sun, 08 Sep 2019 06:14:16 +0000
State-Changed-Why:
I committed a fix, does that fix the problem?


From: Rin Okuyama <rokuyama.rk@gmail.com>
To: gnats-bugs@netbsd.org, abhinav@netbsd.org
Cc: 
Subject: Re: lib/54510 (libedit: filename completion broken in quote/double
 quaote)
Date: Mon, 9 Sep 2019 18:38:55 +0900

 Thank you for working on this. The problems raised in the PR are fixed!

 However, by looking into the code, I noticed some possible problems
 related to unescape_string() function:

 (1) Can we handle \\ (backslash escaped by backslash) correctly?

 (2) In fn_complete(), unescacpe_string() is invoked via
 find_word_to_complete(). After that, the cursor position and end of
 buffer are substituted to *point and *end, respectively, that are
 calculated based on the string before unescaped. Doesn't this break
 3rd party softwares that provide completion_matches and/or
 attempted_completion_function?

 Thanks,
 rin

From: Tom Lane <tgl@sss.pgh.pa.us>
To: gnats-bugs@netbsd.org
Cc: abhinav@netbsd.org, Rin Okuyama <rokuyama.rk@gmail.com>
Subject: Re: lib/54510 (libedit: filename completion broken in quote/double quaote)
Date: Fri, 03 Jan 2020 18:32:39 -0500

 Rin Okuyama <rokuyama.rk@gmail.com> writes:
 > (2) In fn_complete(), unescacpe_string() is invoked via
 > find_word_to_complete(). After that, the cursor position and end of
 > buffer are substituted to *point and *end, respectively, that are
 > calculated based on the string before unescaped. Doesn't this break
 > 3rd party softwares that provide completion_matches and/or
 > attempted_completion_function?

 Indeed, I was about to submit a new bug when I noticed this comment.
 This patch *does* break things for application-specific completion
 functions, because it strips backslashes from the input word whether
 that's appropriate or not.  In the Postgres project's "psql"
 application, one of the things we want to do is tab-complete
 metacommands that start with backslashes, for instance "\pa<TAB>"
 should produce "\password".  That works fine with readline, and it
 worked with libedit up till this patch; but now the completer just
 sees "pa" so it doesn't know what to do.

 Given the precedent of other nearby behaviors, maybe the right
 fix is to skip the unescaping when attempted_completion_function
 is non-NULL.  I haven't tried to code that though.

 (We've worked around this, for the moment, by the expedient of
 ignoring the supplied text altogether in favor of looking into
 rl_line_buffer [1].  But that's an ugly hack so I'd like to
 revert it.)

 			regards, tom lane

 [1] https://git.postgresql.org/gitweb/?p=3Dpostgresql.git;a=3Dcommitdiff;h=
 =3Dddd87d564508bb1c80aac0a4439cfe74a3c203a9

From: Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: NetBSD GNATS <gnats-bugs@netbsd.org>, Abhinav Upadhyay <abhinav@netbsd.org>, 
	Rin Okuyama <rokuyama.rk@gmail.com>
Subject: Re: lib/54510 (libedit: filename completion broken in quote/double quaote)
Date: Sat, 4 Jan 2020 12:39:29 +0530

 On Sat, Jan 4, 2020 at 6:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
 >
 > Rin Okuyama <rokuyama.rk@gmail.com> writes:
 > > (2) In fn_complete(), unescacpe_string() is invoked via
 > > find_word_to_complete(). After that, the cursor position and end of
 > > buffer are substituted to *point and *end, respectively, that are
 > > calculated based on the string before unescaped. Doesn't this break
 > > 3rd party softwares that provide completion_matches and/or
 > > attempted_completion_function?
 >
 > Indeed, I was about to submit a new bug when I noticed this comment.
 > This patch *does* break things for application-specific completion
 > functions, because it strips backslashes from the input word whether
 > that's appropriate or not.  In the Postgres project's "psql"
 > application, one of the things we want to do is tab-complete
 > metacommands that start with backslashes, for instance "\pa<TAB>"
 > should produce "\password".  That works fine with readline, and it
 > worked with libedit up till this patch; but now the completer just
 > sees "pa" so it doesn't know what to do.
 >
 > Given the precedent of other nearby behaviors, maybe the right
 > fix is to skip the unescaping when attempted_completion_function
 > is non-NULL.  I haven't tried to code that though.
 >
 > (We've worked around this, for the moment, by the expedient of
 > ignoring the supplied text altogether in favor of looking into
 > rl_line_buffer [1].  But that's an ugly hack so I'd like to
 > revert it.)
 >
 >                         regards, tom lane
 >
 > [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ddd87d564508bb1c80aac0a4439cfe74a3c203a9

 Can you verify if this patch fixes the problem?

 Index: filecomplete.c
 ===================================================================
 RCS file: /cvsroot/src/lib/libedit/filecomplete.c,v
 retrieving revision 1.62
 diff -u -r1.62 filecomplete.c
 --- filecomplete.c      10 Dec 2019 19:42:09 -0000      1.62
 +++ filecomplete.c      4 Jan 2020 07:07:25 -0000
 @@ -583,10 +583,12 @@

  static wchar_t *
  find_word_to_complete(const wchar_t * cursor, const wchar_t * buffer,
 -    const wchar_t * word_break, const wchar_t * special_prefixes,
 size_t * length)
 +    const wchar_t * word_break, const wchar_t * special_prefixes,
 size_t * length,
 +       int do_unescape)
  {
         /* We now look backwards for the start of a filename/variable word */
         const wchar_t *ctemp = cursor;
 +       wchar_t *temp;
         size_t len;

         /* if the cursor is placed at a slash or a quote, we need to find the
 @@ -629,10 +631,16 @@
                 ctemp++;
         }
         *length = len;
 -       wchar_t *unescaped_word = unescape_string(ctemp, len);
 -       if (unescaped_word == NULL)
 -               return NULL;
 -       return unescaped_word;
 +       if (!do_unescape) {
 +               wchar_t *unescaped_word = unescape_string(ctemp, len);
 +               if (unescaped_word == NULL)
 +                       return NULL;
 +               return unescaped_word;
 +       }
 +       temp = el_malloc((len + 1) * sizeof(*temp));
 +       (void) wcsncpy(temp, ctemp, len);
 +       temp[len] = '\0';
 +       return temp;
  }

  /*
 @@ -662,6 +670,7 @@
         size_t len;
         int what_to_do = '\t';
         int retval = CC_NORM;
 +       int do_unescape = attempted_completion_function == NULL? 1: 0;

         if (el->el_state.lastcmd == el->el_state.thiscmd)
                 what_to_do = '?';
 @@ -677,7 +686,7 @@

         li = el_wline(el);
         temp = find_word_to_complete(li->cursor,
 -           li->buffer, word_break, special_prefixes, &len);
 +           li->buffer, word_break, special_prefixes, &len, do_unescape);
         if (temp == NULL)
                 goto out;

From: Tom Lane <tgl@sss.pgh.pa.us>
To: Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com>
Cc: NetBSD GNATS <gnats-bugs@netbsd.org>,
        Abhinav Upadhyay <abhinav@netbsd.org>,
        Rin Okuyama <rokuyama.rk@gmail.com>
Subject: Re: lib/54510 (libedit: filename completion broken in quote/double quaote)
Date: Sat, 04 Jan 2020 14:06:13 -0500

 Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com> writes:
 > On Sat, Jan 4, 2020 at 6:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
 >> Given the precedent of other nearby behaviors, maybe the right
 >> fix is to skip the unescaping when attempted_completion_function
 >> is non-NULL.  I haven't tried to code that though.

 > Can you verify if this patch fixes the problem?

 I think you have the check backwards: this
 +       if (!do_unescape) {
 should be
 +       if (do_unescape) {

 With that correction, it seems to work for me.

 			regards, tom lane

From: "Abhinav Upadhyay" <abhinav@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54510 CVS commit: src/lib/libedit
Date: Sun, 5 Jan 2020 07:12:05 +0000

 Module Name:	src
 Committed By:	abhinav
 Date:		Sun Jan  5 07:12:05 UTC 2020

 Modified Files:
 	src/lib/libedit: filecomplete.c

 Log Message:
 PR lib/54510 - when user supplied completion function is there,
 don't unescape the string to be completed.


 To generate a diff of this commit:
 cvs rdiff -u -r1.63 -r1.64 src/lib/libedit/filecomplete.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: NetBSD GNATS <gnats-bugs@netbsd.org>, Abhinav Upadhyay <abhinav@netbsd.org>, 
	Rin Okuyama <rokuyama.rk@gmail.com>
Subject: Re: lib/54510 (libedit: filename completion broken in quote/double quaote)
Date: Sun, 5 Jan 2020 13:00:04 +0530

 On Sun, Jan 5, 2020 at 12:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
 >
 > Abhinav Upadhyay <er.abhinav.upadhyay@gmail.com> writes:
 > > On Sat, Jan 4, 2020 at 6:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
 > >> Given the precedent of other nearby behaviors, maybe the right
 > >> fix is to skip the unescaping when attempted_completion_function
 > >> is non-NULL.  I haven't tried to code that though.
 >
 > > Can you verify if this patch fixes the problem?
 >
 > I think you have the check backwards: this
 > +       if (!do_unescape) {
 > should be
 > +       if (do_unescape) {
 >
 > With that correction, it seems to work for me.
 >

 Thanks for the correction and verifying. I committed the change.

State-Changed-From-To: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 31 Dec 2022 06:19:54 +0000
State-Changed-Why:
Feedback received, more commits made, all three years ago. Looks fixed.


>Unformatted:

Home	PR Database Search