Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2 - Mailing list pgsql-bugs

I wrote:
> I'm still of the opinion that
> (1) it's very weird that this code allows for leading space on a line
> but not trailing space;
> (2) we need to look for other places where we have the same issue.
> Possibly libpq is the only chunk of our code that's at serious risk,
> since we don't change the default binary mode in the backend.  But
> even if you assume that that's true, this isn't the only config file
> that libpq examines.

Patch 0001 attached responds to point (1), ie it uses isspace()
tests to get rid of \r and \n and any trailing whitespace in
parseServiceFile().  I think we should do this in HEAD, but there's
perhaps an argument to be made that this is a behavior change and
it'd be better to use Michael's patch in the back branches.

For point (2), I looked through all other fgets() callers in our code.
Not all of them have newline-chomping logic, but I made all the ones
that do have such do it the same way (except for those that use the
isspace() method, which is fine).  I'm not sure if this is fixing any
live bugs --- most of these places are reading popen() output, and
it's unclear to me whether we can rely on that to suppress \r on
Windows.  The Windows-specific code in pipe_read_line seems to think
not (but if its test were dead code we wouldn't know it); yet if this
were a hazard you'd think we'd have gotten complaints about at least
one of these places.  Still, I dislike code that has three or four
randomly different ways of doing the exact same thing, especially when
some of them are gratuitously inefficient.

Note that I standardized on a loop that chomps trailing \r and \n
indiscriminately, not the "if chomp \n then chomp \r" approach we
had in some places.  I think that approach does have a corner-case
bug: if the input is long enough that the \r fits into the buffer
but the \n doesn't, it'd fail to chomp the \r.

Thoughts?

            regards, tom lane

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c800d79..f2b166b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -5020,6 +5020,8 @@ parseServiceFile(const char *serviceFile,

     while ((line = fgets(buf, sizeof(buf), f)) != NULL)
     {
+        int            len;
+
         linenr++;

         if (strlen(line) >= sizeof(buf) - 1)
@@ -5032,16 +5034,17 @@ parseServiceFile(const char *serviceFile,
             return 2;
         }

-        /* ignore EOL at end of line */
-        if (strlen(line) && line[strlen(line) - 1] == '\n')
-            line[strlen(line) - 1] = 0;
+        /* ignore whitespace at end of line, especially the newline */
+        len = strlen(line);
+        while (len > 0 && isspace((unsigned char) line[len - 1]))
+            line[--len] = '\0';

-        /* ignore leading blanks */
+        /* ignore leading whitespace too */
         while (*line && isspace((unsigned char) line[0]))
             line++;

         /* ignore comments and empty lines */
-        if (strlen(line) == 0 || line[0] == '#')
+        if (line[0] == '\0' || line[0] == '#')
             continue;

         /* Check for right groupname */
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 877226d..4abbef5 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -112,9 +112,10 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
         goto error;
     }

-    /* strip trailing newline */
+    /* strip trailing newline, including \r in case we're on Windows */
     len = strlen(buf);
-    if (len > 0 && buf[len - 1] == '\n')
+    while (len > 0 && (buf[len - 1] == '\n' ||
+                       buf[len - 1] == '\r'))
         buf[--len] = '\0';

 error:
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index dfb6c19..79c4627 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2176,6 +2176,7 @@ adjust_data_dir(void)
                 filename[MAXPGPATH],
                *my_exec_path;
     FILE       *fd;
+    int            len;

     /* do nothing if we're working without knowledge of data dir */
     if (pg_config == NULL)
@@ -2218,9 +2219,12 @@ adjust_data_dir(void)
     pclose(fd);
     free(my_exec_path);

-    /* Remove trailing newline */
-    if (strchr(filename, '\n') != NULL)
-        *strchr(filename, '\n') = '\0';
+    /* Remove trailing newline, handling Windows newlines as well */
+    len = strlen(filename);
+    while (len > 0 &&
+           (filename[len - 1] == '\n' ||
+            filename[len - 1] == '\r'))
+        filename[--len] = '\0';

     free(pg_data);
     pg_data = pg_strdup(filename);
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 2734f87..256363c 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -559,12 +559,10 @@ CheckDataVersion(void)

     /* remove trailing newline, handling Windows newlines as well */
     len = strlen(rawline);
-    if (len > 0 && rawline[len - 1] == '\n')
-    {
+    while (len > 0 &&
+           (rawline[len - 1] == '\n' ||
+            rawline[len - 1] == '\r'))
         rawline[--len] = '\0';
-        if (len > 0 && rawline[len - 1] == '\r')
-            rawline[--len] = '\0';
-    }

     if (strcmp(rawline, PG_MAJORVERSION) != 0)
     {
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 73f395f..202da23 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -405,6 +405,7 @@ adjust_data_dir(ClusterInfo *cluster)
                 cmd_output[MAX_STRING];
     FILE       *fp,
                *output;
+    int            len;

     /* Initially assume config dir and data dir are the same */
     cluster->pgconfig = pg_strdup(cluster->pgdata);
@@ -445,9 +446,12 @@ adjust_data_dir(ClusterInfo *cluster)

     pclose(output);

-    /* Remove trailing newline */
-    if (strchr(cmd_output, '\n') != NULL)
-        *strchr(cmd_output, '\n') = '\0';
+    /* Remove trailing newline, handling Windows newlines as well */
+    len = strlen(cmd_output);
+    while (len > 0 &&
+           (cmd_output[len - 1] == '\n' ||
+            cmd_output[len - 1] == '\r'))
+        cmd_output[--len] = '\0';

     cluster->pgdata = pg_strdup(cmd_output);

@@ -508,10 +512,15 @@ get_sock_dir(ClusterInfo *cluster, bool live_check)
                     sscanf(line, "%hu", &old_cluster.port);
                 if (lineno == LOCK_FILE_LINE_SOCKET_DIR)
                 {
+                    int            len;
+
                     cluster->sockdir = pg_strdup(line);
-                    /* strip off newline */
-                    if (strchr(cluster->sockdir, '\n') != NULL)
-                        *strchr(cluster->sockdir, '\n') = '\0';
+                    /* strip off newline, handling Windows newlines as well */
+                    len = strlen(cluster->sockdir);
+                    while (len > 0 &&
+                           (cluster->sockdir[len - 1] == '\n' ||
+                            cluster->sockdir[len - 1] == '\r'))
+                        cluster->sockdir[--len] = '\0';
                 }
             }
             fclose(fp);
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 4514cf8..59afbc7 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -264,6 +264,7 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
                         FILE       *fd;
                         char       *file = pg_strdup(p + 1);
                         int            cmdend;
+                        int            buflen;

                         cmdend = strcspn(file, "`");
                         file[cmdend] = '\0';
@@ -274,8 +275,10 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
                                 buf[0] = '\0';
                             pclose(fd);
                         }
-                        if (strlen(buf) > 0 && buf[strlen(buf) - 1] == '\n')
-                            buf[strlen(buf) - 1] = '\0';
+                        buflen = strlen(buf);
+                        while (buflen > 0 && (buf[buflen - 1] == '\n' ||
+                                              buf[buflen - 1] == '\r'))
+                            buf[--buflen] = '\0';
                         free(file);
                         p += cmdend + 1;
                         break;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c800d79..f2b166b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6910,14 +6913,10 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,

         len = strlen(buf);

-        /* Remove trailing newline */
-        if (len > 0 && buf[len - 1] == '\n')
-        {
+        /* Remove trailing newline, including \r in case we're on Windows */
+        while (len > 0 && (buf[len - 1] == '\n' ||
+                           buf[len - 1] == '\r'))
             buf[--len] = '\0';
-            /* Handle DOS-style line endings, too, even when not on Windows */
-            if (len > 0 && buf[len - 1] == '\r')
-                buf[--len] = '\0';
-        }

         if (len == 0)
             continue;
diff --git a/src/port/sprompt.c b/src/port/sprompt.c
index 146fb00..02164d4 100644
--- a/src/port/sprompt.c
+++ b/src/port/sprompt.c
@@ -144,9 +144,11 @@ simple_prompt(const char *prompt, char *destination, size_t destlen, bool echo)
         } while (buflen > 0 && buf[buflen - 1] != '\n');
     }

-    if (length > 0 && destination[length - 1] == '\n')
-        /* remove trailing newline */
-        destination[length - 1] = '\0';
+    /* strip trailing newline, including \r in case we're on Windows */
+    while (length > 0 &&
+           (destination[length - 1] == '\n' ||
+            destination[length - 1] == '\r'))
+        destination[--length] = '\0';

     if (!echo)
     {