Code review for psql's helpSQL() function.

The loops to identify word boundaries could access past the end of
the input string.  Likely that would never result in an actual
crash, but it makes valgrind unhappy.

The logic to try different numbers of words didn't work when the
input has two words but we only have a match to the first, eg
"\h with select".  (We must "continue" the pass loop, not "break".)

The logic to compute nl_count was bizarrely managed, and in at
least two code paths could end up calling PageOutput with
nl_count = 0, resulting in failing to paginate output that should
have been fed to the pager.  Also, in v12 and up, the nl_count
calculation hadn't been updated to account for the addition of a URL.

The PQExpBuffer holding the command syntax details wasn't freed,
resulting in a session-lifespan memory leak.

While here, improve some comments, choose a more descriptive name
for a variable, fix inconsistent datatype choice for another variable.

Per bug #16837 from Alexander Lakhin.  This code is very old,
so back-patch to all supported branches.

Kyotaro Horiguchi and Tom Lane

Discussion: https://postgr.es/m/16837-479bcd56040c71b3@postgresql.org

diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index ea6cb840815d04ffc2233184f5f23bae05bf31d2..321a333be5a80b5609435fceaed96c8b233480d3 100644 (file)
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -443,6 +443,7 @@ helpSQL(const char *topic, unsigned short int pager)
        int         i;
        int         j;
 
+       /* Find screen width to determine how many columns will fit */
 #ifdef TIOCGWINSZ
        struct winsize screen_size;
 
@@ -480,49 +481,53 @@ helpSQL(const char *topic, unsigned short int pager)
    else
    {
        int         i,
-                   j,
-                   x = 0;
-       bool        help_found = false;
+                   pass;
        FILE       *output = NULL;
        size_t      len,
-                   wordlen;
-       int         nl_count = 0;
+                   wordlen,
+                   j;
+       int         nl_count;
 
        /*
+        * len is the amount of the input to compare to the help topic names.
         * We first try exact match, then first + second words, then first
         * word only.
         */
        len = strlen(topic);
 
-       for (x = 1; x <= 3; x++)
+       for (pass = 1; pass <= 3; pass++)
        {
-           if (x > 1)          /* Nothing on first pass - try the opening
+           if (pass > 1)       /* Nothing on first pass - try the opening
                                 * word(s) */
            {
                wordlen = j = 1;
-               while (topic[j] != ' ' && j++ < len)
+               while (j < len && topic[j++] != ' ')
                    wordlen++;
-               if (x == 2)
+               if (pass == 2 && j < len)
                {
-                   j++;
-                   while (topic[j] != ' ' && j++ <= len)
+                   wordlen++;
+                   while (j < len && topic[j++] != ' ')
                        wordlen++;
                }
-               if (wordlen >= len)     /* Don't try again if the same word */
+               if (wordlen >= len)
                {
-                   if (!output)
-                       output = PageOutput(nl_count, pager ? &(pset.popt.topt) : NULL);
-                   break;
+                   /* Failed to shorten input, so try next pass if any */
+                   continue;
                }
                len = wordlen;
            }
 
-           /* Count newlines for pager */
+           /*
+            * Count newlines for pager.  This logic must agree with what the
+            * following loop will do!
+            */
+           nl_count = 0;
            for (i = 0; QL_HELP[i].cmd; i++)
            {
                if (pg_strncasecmp(topic, QL_HELP[i].cmd, len) == 0 ||
                    strcmp(topic, "*") == 0)
                {
+                   /* magic constant here must match format below! */
                    nl_count += 5 + QL_HELP[i].nl_count;
 
                    /* If we have an exact match, exit.  Fixes \h SELECT */
@@ -530,6 +535,9 @@ helpSQL(const char *topic, unsigned short int pager)
                        break;
                }
            }
+           /* If no matches, don't open the output yet */
+           if (nl_count == 0)
+               continue;
 
            if (!output)
                output = PageOutput(nl_count, pager ? &(pset.popt.topt) : NULL);
@@ -543,24 +551,31 @@ helpSQL(const char *topic, unsigned short int pager)
 
                    initPQExpBuffer(&buffer);
                    QL_HELP[i].syntaxfunc(&buffer);
-                   help_found = true;
+                   /* # of newlines in format must match constant above! */
                    fprintf(output, _("Command:     %s\n"
                                      "Description: %s\n"
                                      "Syntax:\n%s\n\n"),
                            QL_HELP[i].cmd,
                            _(QL_HELP[i].help),
                            buffer.data);
+                   termPQExpBuffer(&buffer);
+
                    /* If we have an exact match, exit.  Fixes \h SELECT */
                    if (pg_strcasecmp(topic, QL_HELP[i].cmd) == 0)
                        break;
                }
            }
-           if (help_found)     /* Don't keep trying if we got a match */
-               break;
+           break;
        }
 
-       if (!help_found)
-           fprintf(output, _("No help available for \"%s\".\nTry \\h with no arguments to see available help.\n"), topic);
+       /* If we never found anything, report that */
+       if (!output)
+       {
+           output = PageOutput(2, pager ? &(pset.popt.topt) : NULL);
+           fprintf(output, _("No help available for \"%s\".\n"
+                             "Try \\h with no arguments to see available help.\n"),
+                   topic);
+       }
 
        ClosePager(output);
    }