From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Jorge Gustavo Rocha <jgr@geomaster.pt>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2
Date: Thu, 27 Jun 2019 12:20:35 -0400
Message-ID: <25605.1561652435@sss.pgh.pa.us> (raw)
In-Reply-To: <3383.1561078113@sss.pgh.pa.us>
References: <82491.1559409563@sss.pgh.pa.us>
<e12a23c6-5e00-3fa6-6179-681336130361@geomaster.pt>
<1473.1560866085@sss.pgh.pa.us>
<fcb5c34c-cf5f-6dc9-c68e-6b297f073b79@geomaster.pt>
<CA+mi_8aBnLKypX2Ttksg8JEhApBo=JpF6P_STQGp-FOvDpn6Yw@mail.gmail.com>
<81d9d421-6947-7111-a1dc-cfd99cd3ae53@geomaster.pt>
<23215.1560977098@sss.pgh.pa.us>
<20190620031218.GC2660@paquier.xyz>
<20190620070319.GH2660@paquier.xyz>
<0a9fcd30-5ae2-7e5f-ac1d-60d14a2ebb63@geomaster.pt>
<20190621004233.GA1626@paquier.xyz>
<3383.1561078113@sss.pgh.pa.us>
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
Attachments:
[text/x-diff] 0001-fix-parseServiceFile.patch (1.1K, ../25605.1561652435@sss.pgh.pa.us/2-0001-fix-parseServiceFile.patch)
download | inline diff:diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.cindex 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 */
[text/x-diff] 0002-fix-other-places.patch (5.6K, ../25605.1561652435@sss.pgh.pa.us/3-0002-fix-other-places.patch)
download | inline diff:diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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)
{
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, michael@paquier.xyz, jgr@geomaster.pt, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2
In-Reply-To: <25605.1561652435@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox