Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ikvGm-0005Y6-FI for pgsql-hackers@arkaria.postgresql.org; Fri, 27 Dec 2019 19:27:32 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1ikvGk-0000pK-Pd for pgsql-hackers@arkaria.postgresql.org; Fri, 27 Dec 2019 19:27:30 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ikvGk-0000pC-FE for pgsql-hackers@lists.postgresql.org; Fri, 27 Dec 2019 19:27:30 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ikvGg-0002Sh-Nn for pgsql-hackers@lists.postgresql.org; Fri, 27 Dec 2019 19:27:30 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id xBRJRHR1028923; Fri, 27 Dec 2019 14:27:17 -0500 From: Tom Lane To: Alvaro Herrera cc: pgsql-hackers@lists.postgresql.org, steven.winfield@cantabcapital.com Subject: Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes In-reply-to: <18490.1576264601@sss.pgh.pa.us> References: <20191211015552.GA6301@alvherre.pgsql> <18490.1576264601@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 13 Dec 2019 14:16:41 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <28896.1577474814.0@sss.pgh.pa.us> Date: Fri, 27 Dec 2019 14:27:17 -0500 Message-ID: <28922.1577474837@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <28896.1577474814.1@sss.pgh.pa.us> I wrote: > [ psql-filename-completion-fixes-2.patch ] The cfbot noted this was broken by the removal of pg_config.h.win32, so here's a new version rebased over that. No changes other than adjusting the MSVC autoconf-substitute code. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="psql-filename-completion-fixes-3.patch"; charset="us-ascii" Content-ID: <28896.1577474814.2@sss.pgh.pa.us> Content-Description: psql-filename-completion-fixes-3.patch Content-Transfer-Encoding: quoted-printable diff --git a/config/programs.m4 b/config/programs.m4 index 90ff944..68ab823 100644 --- a/config/programs.m4 +++ b/config/programs.m4 @@ -209,17 +209,20 @@ fi = = = -# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER -# --------------------------------------- +# PGAC_READLINE_VARIABLES +# ----------------------- # Readline versions < 2.1 don't have rl_completion_append_character +# Libedit lacks rl_filename_quote_characters and rl_filename_quoting_func= tion = -AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER], +AC_DEFUN([PGAC_READLINE_VARIABLES], [AC_CACHE_CHECK([for rl_completion_append_character], pgac_cv_var_rl_comp= letion_append_character, [AC_LINK_IFELSE([AC_LANG_PROGRAM([#include -#ifdef HAVE_READLINE_READLINE_H -# include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include #elif defined(HAVE_READLINE_H) -# include +#include #endif ], [rl_completion_append_character =3D 'x';])], @@ -228,7 +231,42 @@ AC_DEFUN([PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER], if test x"$pgac_cv_var_rl_completion_append_character" =3D x"yes"; then AC_DEFINE(HAVE_RL_COMPLETION_APPEND_CHARACTER, 1, [Define to 1 if you have the global variable 'rl_completion_app= end_character'.]) -fi])# PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER +fi +AC_CACHE_CHECK([for rl_filename_quote_characters], pgac_cv_var_rl_filenam= e_quote_characters, +[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include +#elif defined(HAVE_READLINE_H) +#include +#endif +], +[rl_filename_quote_characters =3D "x";])], +[pgac_cv_var_rl_filename_quote_characters=3Dyes], +[pgac_cv_var_rl_filename_quote_characters=3Dno])]) +if test x"$pgac_cv_var_rl_filename_quote_characters" =3D x"yes"; then +AC_DEFINE(HAVE_RL_FILENAME_QUOTE_CHARACTERS, 1, + [Define to 1 if you have the global variable 'rl_filename_quote= _characters'.]) +fi +AC_CACHE_CHECK([for rl_filename_quoting_function], pgac_cv_var_rl_filenam= e_quoting_function, +[AC_LINK_IFELSE([AC_LANG_PROGRAM([#include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include +#elif defined(HAVE_READLINE_H) +#include +#endif +], +[rl_filename_quoting_function =3D 0;])], +[pgac_cv_var_rl_filename_quoting_function=3Dyes], +[pgac_cv_var_rl_filename_quoting_function=3Dno])]) +if test x"$pgac_cv_var_rl_filename_quoting_function" =3D x"yes"; then +AC_DEFINE(HAVE_RL_FILENAME_QUOTING_FUNCTION, 1, + [Define to 1 if you have the global variable 'rl_filename_quoti= ng_function'.]) +fi +])# PGAC_READLINE_VARIABLES = = = diff --git a/configure b/configure index 9de5037..994e736 100755 --- a/configure +++ b/configure @@ -16314,10 +16314,12 @@ else cat confdefs.h - <<_ACEOF >conftest.$ac_ext /* end confdefs.h. */ #include -#ifdef HAVE_READLINE_READLINE_H -# include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include #elif defined(HAVE_READLINE_H) -# include +#include #endif = int @@ -16343,6 +16345,85 @@ if test x"$pgac_cv_var_rl_completion_append_chara= cter" =3D x"yes"; then $as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h = fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quote_c= haracters" >&5 +$as_echo_n "checking for rl_filename_quote_characters... " >&6; } +if ${pgac_cv_var_rl_filename_quote_characters+:} false; then : + $as_echo_n "(cached) " >&6 +else + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include +#elif defined(HAVE_READLINE_H) +#include +#endif + +int +main () +{ +rl_filename_quote_characters =3D "x"; + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO"; then : + pgac_cv_var_rl_filename_quote_characters=3Dyes +else + pgac_cv_var_rl_filename_quote_characters=3Dno +fi +rm -f core conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename= _quote_characters" >&5 +$as_echo "$pgac_cv_var_rl_filename_quote_characters" >&6; } +if test x"$pgac_cv_var_rl_filename_quote_characters" =3D x"yes"; then + +$as_echo "#define HAVE_RL_FILENAME_QUOTE_CHARACTERS 1" >>confdefs.h + +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for rl_filename_quoting= _function" >&5 +$as_echo_n "checking for rl_filename_quoting_function... " >&6; } +if ${pgac_cv_var_rl_filename_quoting_function+:} false; then : + $as_echo_n "(cached) " >&6 +else + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include +#if defined(HAVE_READLINE_READLINE_H) +#include +#elif defined(HAVE_EDITLINE_READLINE_H) +#include +#elif defined(HAVE_READLINE_H) +#include +#endif + +int +main () +{ +rl_filename_quoting_function =3D 0; + ; + return 0; +} +_ACEOF +if ac_fn_c_try_link "$LINENO"; then : + pgac_cv_var_rl_filename_quoting_function=3Dyes +else + pgac_cv_var_rl_filename_quoting_function=3Dno +fi +rm -f core conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_var_rl_filename= _quoting_function" >&5 +$as_echo "$pgac_cv_var_rl_filename_quoting_function" >&6; } +if test x"$pgac_cv_var_rl_filename_quoting_function" =3D x"yes"; then + +$as_echo "#define HAVE_RL_FILENAME_QUOTING_FUNCTION 1" >>confdefs.h + +fi + for ac_func in rl_completion_matches rl_filename_completion_function rl= _reset_screen_size do : as_ac_var=3D`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` diff --git a/configure.in b/configure.in index 9c5e5e7..0921262 100644 --- a/configure.in +++ b/configure.in @@ -1873,7 +1873,7 @@ fi LIBS=3D"$LIBS_including_readline" = if test "$with_readline" =3D yes; then - PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER + PGAC_READLINE_VARIABLES AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function r= l_reset_screen_size]) AC_CHECK_FUNCS([append_history history_truncate_file]) fi diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c index 8c8b4c2..23a6f9f 100644 --- a/src/bin/psql/stringutils.c +++ b/src/bin/psql/stringutils.c @@ -282,6 +282,7 @@ strip_quotes(char *source, char quote, char escape, in= t encoding) * entails_quote - any of these present? need outer quotes * quote - doubled within string, affixed to both ends * escape - doubled within string + * force_quote - if true, quote the output even if it doesn't "need" it * encoding - the active character-set encoding * * Do not use this as a substitute for PQescapeStringConn(). Use it for @@ -289,12 +290,13 @@ strip_quotes(char *source, char quote, char escape, = int encoding) */ char * quote_if_needed(const char *source, const char *entails_quote, - char quote, char escape, int encoding) + char quote, char escape, bool force_quote, + int encoding) { const char *src; char *ret; char *dst; - bool need_quotes =3D false; + bool need_quotes =3D force_quote; = Assert(source !=3D NULL); Assert(quote !=3D '\0'); diff --git a/src/bin/psql/stringutils.h b/src/bin/psql/stringutils.h index 16a7af0..fa00141 100644 --- a/src/bin/psql/stringutils.h +++ b/src/bin/psql/stringutils.h @@ -22,6 +22,7 @@ extern char *strtokx(const char *s, extern void strip_quotes(char *source, char quote, char escape, int encod= ing); = extern char *quote_if_needed(const char *source, const char *entails_quot= e, - char quote, char escape, int encoding); + char quote, char escape, bool force_quote, + int encoding); = #endif /* STRINGUTILS_H */ diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 5e0db35..dc867bd 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -41,6 +41,7 @@ #ifdef USE_READLINE = #include +#include = #include "catalog/pg_am_d.h" #include "catalog/pg_class_d.h" @@ -63,6 +64,15 @@ #define rl_completion_matches completion_matches #endif = +/* + * Currently we assume that rl_filename_dequoting_function exists if + * rl_filename_quoting_function does. If that proves not to be the case, + * we'd need to test for the former, or possibly both, in configure. + */ +#ifdef HAVE_RL_FILENAME_QUOTING_FUNCTION +#define USE_FILENAME_QUOTING_FUNCTIONS 1 +#endif + /* word break characters */ #define WORD_BREAKS "\t\n@$><=3D;|&{() " = @@ -1114,9 +1124,9 @@ static char **get_previous_words(int point, char **b= uffer, int *nwords); = static char *get_guctype(const char *varname); = -#ifdef NOT_USED -static char *quote_file_name(char *text, int match_type, char *quote_poin= ter); -static char *dequote_file_name(char *text, char quote_char); +#ifdef USE_FILENAME_QUOTING_FUNCTIONS +static char *quote_file_name(char *fname, int match_type, char *quote_poi= nter); +static char *dequote_file_name(char *fname, int quote_char); #endif = = @@ -1129,8 +1139,32 @@ initialize_readline(void) rl_readline_name =3D (char *) pset.progname; rl_attempted_completion_function =3D psql_completion; = +#ifdef USE_FILENAME_QUOTING_FUNCTIONS + rl_filename_quoting_function =3D quote_file_name; + rl_filename_dequoting_function =3D dequote_file_name; +#endif + rl_basic_word_break_characters =3D WORD_BREAKS; = + rl_completer_quote_characters =3D "'\""; + + /* + * Set rl_filename_quote_characters to "all possible characters", + * otherwise Readline will skip filename quoting if it thinks a filename + * doesn't need quoting. Readline actually interprets this as bytes, so + * there are no encoding considerations here. + */ +#ifdef HAVE_RL_FILENAME_QUOTE_CHARACTERS + { + unsigned char *fqc =3D (unsigned char *) pg_malloc(256); + + for (int i =3D 0; i < 255; i++) + fqc[i] =3D (unsigned char) (i + 1); + fqc[255] =3D '\0'; + rl_filename_quote_characters =3D (const char *) fqc; + } +#endif + completion_max_records =3D 1000; = /* @@ -4378,15 +4412,49 @@ complete_from_variables(const char *text, const ch= ar *prefix, const char *suffix = /* * This function wraps rl_filename_completion_function() to strip quotes = from - * the input before searching for matches and to quote any matches for wh= ich - * the consuming command will require it. + * the input before searching for matches and then re-quote the matches. + * (We always quote filenames, even though for some psql meta-commands an= d + * some filenames it wouldn't be strictly necessary.) + * + * Caller must set completion_charp to a zero- or one-character string + * containing the escape character. This is necessary since \copy has no + * escape character, but every other backslash command recognizes "\" as = an + * escape character. Since we have only two callers, don't bother provid= ing + * a macro to simplify this. */ static char * complete_from_files(const char *text, int state) { +#ifdef USE_FILENAME_QUOTING_FUNCTIONS + /* + * If we're using a version of Readline that supports filename quoting + * hooks, rely on those, and invoke rl_filename_completion_function() + * without messing with its arguments. Readline does stuff internally + * that does not work well at all if we try to handle dequoting here. + * Instead, Readline will call quote_file_name() and dequote_file_name() + * (see below) at appropriate times. + * + * ... or at least, mostly it will. There are some paths involving + * unmatched file names in which Readline never calls quote_file_name(), + * and if left to its own devices it will incorrectly append a quote + * anyway. Set rl_completion_suppress_quote to prevent that. If we do + * get to quote_file_name(), we'll clear this again. (Yes, this seems + * like it's working around Readline bugs.) + * + * (For now, we assume that rl_completion_suppress_quote exists if the + * filename quoting hooks do.) + */ + rl_completion_suppress_quote =3D 1; + + return rl_filename_completion_function(text, state); +#else + /* + * Otherwise, we have to do the best we can. + */ static const char *unquoted_text; char *unquoted_match; char *ret =3D NULL; + struct stat statbuf; = if (state =3D=3D 0) { @@ -4404,22 +4472,36 @@ complete_from_files(const char *text, int state) unquoted_match =3D rl_filename_completion_function(unquoted_text, state)= ; if (unquoted_match) { + /* Re-quote the result. */ + ret =3D quote_if_needed(unquoted_match, " \t\r\n\"`", + '\'', *completion_charp, + true, + pset.encoding); + Assert(ret); + /* - * Caller sets completion_charp to a zero- or one-character string - * containing the escape character. This is necessary since \copy has - * no escape character, but every other backslash command recognizes - * "\" as an escape character. Since we have only two callers, don't - * bother providing a macro to simplify this. + * If it's a directory, replace trailing quote with a slash; this is + * usually more convenient. */ - ret =3D quote_if_needed(unquoted_match, " \t\r\n\"`", - '\'', *completion_charp, pset.encoding); - if (ret) - free(unquoted_match); - else - ret =3D unquoted_match; + if (*ret && + stat(unquoted_match, &statbuf) =3D=3D 0 && + S_ISDIR(statbuf.st_mode)) + { + char *retend =3D ret + strlen(ret) - 1; + + Assert(*retend =3D=3D '\''); + *retend =3D '/'; + /* Try to prevent libedit from adding a space, too */ +#ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER + rl_completion_append_character =3D '\0'; +#endif + } + + free(unquoted_match); } = return ret; +#endif /* USE_FILENAME_QUOTING_FUNCTIONS */ } = = @@ -4671,46 +4753,104 @@ get_guctype(const char *varname) return guctype; } = -#ifdef NOT_USED +#ifdef USE_FILENAME_QUOTING_FUNCTIONS = /* - * Surround a string with single quotes. This works for both SQL and - * psql internal. Currently disabled because it is reported not to - * cooperate with certain versions of readline. + * Quote a filename according to SQL rules, returning a malloc'd string. + * completion_charp must point to escape character or '\0', as per + * comments for complete_from_files(). */ static char * -quote_file_name(char *text, int match_type, char *quote_pointer) +quote_file_name(char *fname, int match_type, char *quote_pointer) { char *s; - size_t length; + struct stat statbuf; = - (void) quote_pointer; /* not used */ + /* We always quote (so quote_if_needed's API is overkill) */ + s =3D quote_if_needed(fname, " \t\r\n\"`", + '\'', *completion_charp, + true, + pset.encoding); + + /* + * However, some of the time we have to strip the trailing quote from wh= at + * we send back. The tests on "s" are just paranoia given that we force= d + * quoting. The meat of this is that we suppress the trailing quote if = we + * have multiple/no matches (because we don't want to add a quote if the + * input is seemingly unfinished), or if the input was already quoted + * (because Readline will do arguably-buggy things otherwise), or if the + * file does not exist, or if it's a directory. + */ + if (s && *s && + (match_type !=3D SINGLE_MATCH || + (quote_pointer && *quote_pointer =3D=3D '\'') || + stat(fname, &statbuf) !=3D 0 || + S_ISDIR(statbuf.st_mode))) + { + char *send =3D s + strlen(s) - 1; + + Assert(*send =3D=3D '\''); + *send =3D '\0'; + } + + /* + * And now we can let Readline do its thing with possibly adding a quote + * on its own accord. (This covers some additional cases beyond those + * dealt with above.) + */ + rl_completion_suppress_quote =3D 0; + + /* + * If user typed a leading quote character other than single quote (i.e.= , + * double quote), zap it, so that we replace it with the correct single + * quote. + */ + if (quote_pointer && *quote_pointer !=3D '\'') + *quote_pointer =3D '\0'; = - length =3D strlen(text) +(match_type =3D=3D SINGLE_MATCH ? 3 : 2); - s =3D pg_malloc(length); - s[0] =3D '\''; - strcpy(s + 1, text); - if (match_type =3D=3D SINGLE_MATCH) - s[length - 2] =3D '\''; - s[length - 1] =3D '\0'; return s; } = +/* + * Dequote a filename, if it's quoted. + * completion_charp must point to escape character or '\0', as per + * comments for complete_from_files(). + */ static char * -dequote_file_name(char *text, char quote_char) +dequote_file_name(char *fname, int quote_char) { - char *s; - size_t length; + char *unquoted_fname; + + /* + * If quote_char is set, it's not included in "fname". We have to add i= t + * or strtokx will not interpret the string correctly (notably, it won't + * recognize escapes). + */ + if (quote_char =3D=3D '\'') + { + char *workspace =3D (char *) pg_malloc(strlen(fname) + 2); = - if (!quote_char) - return pg_strdup(text); + workspace[0] =3D quote_char; + strcpy(workspace + 1, fname); + unquoted_fname =3D strtokx(workspace, "", NULL, "'", *completion_charp, + false, true, pset.encoding); + free(workspace); + } + else + unquoted_fname =3D strtokx(fname, "", NULL, "'", *completion_charp, + false, true, pset.encoding); = - length =3D strlen(text); - s =3D pg_malloc(length - 2 + 1); - strlcpy(s, text +1, length - 2 + 1); + /* expect a NULL return for the empty string only */ + if (!unquoted_fname) + { + Assert(*fname =3D=3D '\0'); + unquoted_fname =3D fname; + } = - return s; + /* readline expects a malloc'd result that it is to free */ + return pg_strdup(unquoted_fname); } -#endif /* NOT_USED */ + +#endif /* USE_FILENAME_QUOTING_FUNCTIONS */ = #endif /* USE_READLINE */ diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 050c48b..983d94e 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -488,6 +488,14 @@ /* Define to 1 if you have the `rl_filename_completion_function' function= . */ #undef HAVE_RL_FILENAME_COMPLETION_FUNCTION = +/* Define to 1 if you have the global variable 'rl_filename_quote_charact= ers'. + */ +#undef HAVE_RL_FILENAME_QUOTE_CHARACTERS + +/* Define to 1 if you have the global variable 'rl_filename_quoting_funct= ion'. + */ +#undef HAVE_RL_FILENAME_QUOTING_FUNCTION + /* Define to 1 if you have the `rl_reset_screen_size' function. */ #undef HAVE_RL_RESET_SCREEN_SIZE = diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 909bded..979964c 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -333,6 +333,8 @@ sub GenerateFiles HAVE_RL_COMPLETION_APPEND_CHARACTER =3D> undef, HAVE_RL_COMPLETION_MATCHES =3D> undef, HAVE_RL_FILENAME_COMPLETION_FUNCTION =3D> undef, + HAVE_RL_FILENAME_QUOTE_CHARACTERS =3D> undef, + HAVE_RL_FILENAME_QUOTING_FUNCTION =3D> undef, HAVE_RL_RESET_SCREEN_SIZE =3D> undef, HAVE_SECURITY_PAM_APPL_H =3D> undef, HAVE_SETPROCTITLE =3D> undef, ------- =_aaaaaaaaaa0--