Minor issues in .pgpass

On 21 Jan 2020, at 07:27, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

For (2), libpq should treat any lines beginning with # as comments.

I haven't read the code to confirm that it really isn't, but +1 on making it
so. I can't see a reason for allowing a hostname to start with #, but allowing
comments does seem useful.

cheers ./daniel

On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote:

Hi,

When I was researching the maximum length of password in PostgreSQL
to answer the question from my customer, I found that there are two
minor issues in .pgpass file.

(1) If the length of a line in .pgpass file is larger than 319B,
libpq silently treats each 319B in the line as a separate
setting line.

This seems like a potentially serious bug. For example, a truncated
password could get retried enough times to raise intruder alarms, and
it wouldn't be easy to track down.

(2) The document explains that a line beginning with # is treated
as a comment in .pgpass. But as far as I read the code,
there is no code doing such special handling.

This is a flat-out bug, as it violates a promise the documentation has
made.

Also if the length of that "comment" line is larger than 319B,
the latter part of the line can be treated as valid setting.

You may think that these unexpected behaviors are not so harmful
in practice because "usually" the length of password setting line is
less than 319B and the hostname beginning with # is less likely to be
used. But the problem exists. And there are people who want to use
large password or to write a long comment (e.g., with multibyte
characters like Japanese) in .pgass, so these may be more harmful
in the near future.

For (1), I think that we should make libpq warn if the length of a line
is larger than 319B, and throw away the remaining part beginning from
320B position. Whether to enlarge the length of a line should be
a separate discussion, I think.

Agreed.

For (2), libpq should treat any lines beginning with # as comments.

Would it make sense for lines starting with whitespace and then # to
be treated as comments, too, e.g.:

# Please don't treat this as a parameter

?

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 2020/01/22 9:06, David Fetter wrote:

On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote:

Hi,

When I was researching the maximum length of password in PostgreSQL
to answer the question from my customer, I found that there are two
minor issues in .pgpass file.

(1) If the length of a line in .pgpass file is larger than 319B,
libpq silently treats each 319B in the line as a separate
setting line.

This seems like a potentially serious bug. For example, a truncated
password could get retried enough times to raise intruder alarms, and
it wouldn't be easy to track down.

(2) The document explains that a line beginning with # is treated
as a comment in .pgpass. But as far as I read the code,
there is no code doing such special handling.

This is a flat-out bug, as it violates a promise the documentation has
made.

Also if the length of that "comment" line is larger than 319B,
the latter part of the line can be treated as valid setting.

You may think that these unexpected behaviors are not so harmful
in practice because "usually" the length of password setting line is
less than 319B and the hostname beginning with # is less likely to be
used. But the problem exists. And there are people who want to use
large password or to write a long comment (e.g., with multibyte
characters like Japanese) in .pgass, so these may be more harmful
in the near future.

For (1), I think that we should make libpq warn if the length of a line
is larger than 319B, and throw away the remaining part beginning from
320B position. Whether to enlarge the length of a line should be
a separate discussion, I think.

Agreed.

For (2), libpq should treat any lines beginning with # as comments.

Patch attached. This patch does the above (1) and (2).

Would it make sense for lines starting with whitespace and then # to
be treated as comments, too, e.g.:

Could you tell me why you want to treat such a line as comment?
Basically I don't want to change the existing rules for parsing
.pgpass file more thane necessary.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem.

I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.

1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables. Also, I would choose a more appropriate name for "tmp" variable.

I believe if you move the following lines before the conditional statement and simply and change the if statement to "if (len >= sizeof(buf) - 1)", it will serve the purpose.
========================================
/* strip trailing newline and carriage return */
len = pg_strip_crlf(buf);

if (len == 0)
continue;
========================================

So, the patch should look like this in my opinion (ignore the formatting issues as this is just to give you an idea of what I mean):

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 408000a..6ca262f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6949,6 +6949,7 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
 {
        FILE       *fp;
        struct stat stat_buf;
+       int             line_number = 0;

#define LINELEN NAMEDATALEN*5
char buf[LINELEN];
@@ -7018,10 +7019,40 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
if (fgets(buf, sizeof(buf), fp) == NULL)
break;

-               /* strip trailing newline and carriage return */
-               len = pg_strip_crlf(buf);
+               line_number++;

-               if (len == 0)
+                /* strip trailing newline and carriage return */
+                len = pg_strip_crlf(buf);
+
+                if (len == 0)
+                        continue;
+
+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];
+
+                       /*
+                        * Warn if this password setting line is too long,
+                        * because it's unexpectedly truncated.
+                        */
+                       if (buf[0] != '#')
+                               fprintf(stderr,
+                                               libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"),
+                                               line_number, pgpassfile);
+
+                       /* eat rest of the line */
+                       while (!feof(fp) && !ferror(fp))
+                       {
+                               if (fgets(tmp, sizeof(tmp), fp) == NULL)
+                                       break;
+                               len = strlen(tmp);
+                               if (len < sizeof(tmp) -1 || tmp[len - 1] == '\n')
+                                       break;
+                       }
+               }
+
+               /* ignore comments */
+               if (buf[0] == '#')

---
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950  EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

The new status of this patch is: Waiting on Author

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem.

I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting. Please have a look and resolve those.

Yes.

2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

I believe if you move the following lines before the conditional statement and simply and change the if statement to "if (len >= sizeof(buf) - 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

First of all, this seems like fixing a valid issue, albeit, the

probability of somebody messing is low, but it is still better to fix this
problem.

I've not tested the patch in any detail, however, there are a couple of

comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting. Please have a look

and resolve those.

Yes.

2. I think you can potentially use "len" variable instead of introducing

"buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

I believe if you move the following lines before the conditional

statement and simply and change the if statement to "if (len >= sizeof(buf)
- 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"

If the buf read contains a newline or a carriage return at the end, then
clearly the line
is not exceeding the sizeof(buf). If alternatively, it doesn't, then
pg_strip_crlf will have
no effect on string length and for any lines exceeding sizeof(buf), the
following conditional
statement becomes true.

+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar <hamid.akhtar@gmail.com> wrote:

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

First of all, this seems like fixing a valid issue, albeit, the

probability of somebody messing is low, but it is still better to fix this
problem.

I've not tested the patch in any detail, however, there are a couple of

comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting. Please have a look

and resolve those.

Yes.

2. I think you can potentially use "len" variable instead of

introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

That is fine.

Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

May be something like "excess_buf" or any other one that describes that
these bytes are to be discarded.

I believe if you move the following lines before the conditional

statement and simply and change the if statement to "if (len >= sizeof(buf)
- 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"

If the buf read contains a newline or a carriage return at the end, then
clearly the line
is not exceeding the sizeof(buf). If alternatively, it doesn't, then
pg_strip_crlf will have
no effect on string length and for any lines exceeding sizeof(buf), the
following conditional
statement becomes true.

+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

On 2020/03/03 21:38, Hamid Akhtar wrote:

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem.

I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting. Please have a look and resolve those.

Yes.

2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

I believe if you move the following lines before the conditional statement and simply and change the if statement to "if (len >= sizeof(buf) - 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"
If the buf read contains a newline or a carriage return at the end, then clearly the line
is not exceeding the sizeof(buf).

No if the length of the setting line exceeds sizeof(buf) and
the buf contains only a carriage return at the end and not newline.
This case can happen because fgets() stops reading when a newline
(not a carriage return) is found. Normal users are very unlikely to
add a carriage return into the middle of the pgpass setting line
in practice, though. But IMO the code should handle even this
case because it *can* happen, if the code is not so complicated.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On 2020/03/03 22:07, Hamid Akhtar wrote:

On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar <hamid.akhtar@gmail.com <mailto:hamid.akhtar@gmail.com>> wrote:

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem.

I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting. Please have a look and resolve those.

Yes.

Fixed. Attached is the updated version of the patch.
I marked this CF entry as "Needs Review" again.

2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

That is fine.

Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

May be something like "excess_buf" or any other one that describes that these bytes are to be discarded.

Thanks for the comment! But IMO that "rest" is not
so bad choice, so for now I used "rest" in the latest patch.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2020/03/03 21:38, Hamid Akhtar wrote:

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com

<mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the commitfest

application:

make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

First of all, this seems like fixing a valid issue, albeit, the

probability of somebody messing is low, but it is still better to fix this
problem.

I've not tested the patch in any detail, however, there are a

couple of comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting. Please have

a look and resolve those.

Yes.

2. I think you can potentially use "len" variable instead of

introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

I believe if you move the following lines before the conditional

statement and simply and change the if statement to "if (len >= sizeof(buf)
- 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too

long

so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"
If the buf read contains a newline or a carriage return at the end, then

clearly the line

is not exceeding the sizeof(buf).

No if the length of the setting line exceeds sizeof(buf) and
the buf contains only a carriage return at the end and not newline.
This case can happen because fgets() stops reading when a newline
(not a carriage return) is found. Normal users are very unlikely to
add a carriage return into the middle of the pgpass setting line
in practice, though. But IMO the code should handle even this
case because it *can* happen, if the code is not so complicated.

I'm not sure if I understand your comment here. From the code of
pg_strip_crlf
I see that it is handling both carriage return and/or new line at the end
of a
string:
=============
src/common/string.c
=============
while (len > 0 && (str[len - 1] == '\n' || str[len - 1] == '\r'))
str[--len] = '\0';
=============

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

On 2020/03/04 20:39, Hamid Akhtar wrote:

On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2020/03/03 21:38, Hamid Akhtar wrote:

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com> <mailto:masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>>> wrote:

     On 2020/02/29 0:46, Hamid Akhtar wrote:
      > The following review has been posted through the commitfest application:
      > make installcheck-world:  not tested
      > Implements feature:       not tested
      > Spec compliant:           not tested
      > Documentation:            not tested
      >
      > First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem.
      >
      > I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.

     Thanks for the review and comments!

      > 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.

     Yes.

      > 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.

     Basically I don't want to use the same variable for several purposes
     because which would decrease the code readability.

      > Also, I would choose a more appropriate name for "tmp" variable.

     Yeah, so what about "rest" as the variable name?

      > I believe if you move the following lines before the conditional statement and simply and change the if statement to "if (len >= sizeof(buf) - 1)", it will serve the purpose.

     ISTM that this doesn't work correctly when the "buf" contains
     trailing carriage returns but not newlines (i.e., this line is too long
     so the "buf" doesn't include newline). In this case, pg_strip_crlf()
     shorten the "buf" and then its return value "len" should become
     less than sizeof(buf). So the following condition always becomes
     false unexpectedly in that case even though there is still rest of
     the line to eat.

Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"
If the buf read contains a newline or a carriage return at the end, then clearly the line
is not exceeding the sizeof(buf).

No if the length of the setting line exceeds sizeof(buf) and
the buf contains only a carriage return at the end and not newline.
This case can happen because fgets() stops reading when a newline
(not a carriage return) is found. Normal users are very unlikely to
add a carriage return into the middle of the pgpass setting line
in practice, though. But IMO the code should handle even this
case because it *can* happen, if the code is not so complicated.

I'm not sure if I understand your comment here. From the code of pg_strip_crlf
I see that it is handling both carriage return and/or new line at the end of a
string:

So if "buf" contains a carriage return at the end, it's removed and
the "len" that pg_strip_crlf() returns obviously should be smaller
than sizeof(buf). This causes the following condition that you
proposed as follows to always be false (i.e., len < sizeof(buf) - 1)
even when there are still rest of line. So we cannot eat rest of
the line even though it exists. I'm missing something?

+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];
+
+                       /*
+                        * Warn if this password setting line is too long,
+                        * because it's unexpectedly truncated.
+                        */
+                       if (buf[0] != '#')
+                               fprintf(stderr,
+                                               libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"),
+                                               line_number, pgpassfile);
+
+                       /* eat rest of the line */
+                       while (!feof(fp) && !ferror(fp))

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Wed, Mar 4, 2020 at 4:54 PM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2020/03/04 20:39, Hamid Akhtar wrote:

On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com

<mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2020/03/03 21:38, Hamid Akhtar wrote:

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <

masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com> <mailto:
masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>>> wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:

The following review has been posted through the

commitfest application:

make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

First of all, this seems like fixing a valid issue,

albeit, the probability of somebody messing is low, but it is still better
to fix this problem.

I've not tested the patch in any detail, however, there

are a couple of comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

1. pgindent is showing a few issues with formatting.

Please have a look and resolve those.

Yes.

2. I think you can potentially use "len" variable instead

of introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several

purposes

because which would decrease the code readability.

Also, I would choose a more appropriate name for "tmp"

variable.

Yeah, so what about "rest" as the variable name?

I believe if you move the following lines before the

conditional statement and simply and change the if statement to "if (len >=
sizeof(buf) - 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line

is too long

so the "buf" doesn't include newline). In this case,

pg_strip_crlf()

shorten the "buf" and then its return value "len" should

become

less than sizeof(buf). So the following condition always

becomes

false unexpectedly in that case even though there is still

rest of

the line to eat.

Per code comments for pg_strip_crlf:
"pg_strip_crlf -- Remove any trailing newline and carriage return"
If the buf read contains a newline or a carriage return at the

end, then clearly the line

is not exceeding the sizeof(buf).

No if the length of the setting line exceeds sizeof(buf) and
the buf contains only a carriage return at the end and not newline.
This case can happen because fgets() stops reading when a newline
(not a carriage return) is found. Normal users are very unlikely to
add a carriage return into the middle of the pgpass setting line
in practice, though. But IMO the code should handle even this
case because it *can* happen, if the code is not so complicated.

I'm not sure if I understand your comment here. From the code of

pg_strip_crlf

I see that it is handling both carriage return and/or new line at the

end of a

string:

So if "buf" contains a carriage return at the end, it's removed and
the "len" that pg_strip_crlf() returns obviously should be smaller
than sizeof(buf). This causes the following condition that you
proposed as follows to always be false (i.e., len < sizeof(buf) - 1)
even when there are still rest of line. So we cannot eat rest of
the line even though it exists. I'm missing something?

No, you are perfectly fine. I now understand where you are coming from. So,
all good now.

+               if (len >= sizeof(buf) - 1)
+               {
+                       char    tmp[LINELEN];
+
+                       /*
+                        * Warn if this password setting line is too long,
+                        * because it's unexpectedly truncated.
+                        */
+                       if (buf[0] != '#')
+                               fprintf(stderr,
+                                               libpq_gettext("WARNING:
line %d too long in password file \"%s\"\n"),
+                                               line_number, pgpassfile);
+
+                       /* eat rest of the line */
+                       while (!feof(fp) && !ferror(fp))

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Tested and looks fine to me.

The new status of this patch is: Ready for Committer

On 2020/03/04 23:01, Hamid Akhtar wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Tested and looks fine to me.

The new status of this patch is: Ready for Committer

Many thanks for testing and reviewing the patch!
I pushed it.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters