Re: Minor issues in .pgpass

[Fujii Masao]

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

Re: Minor issues in .pgpass

[Hamid Akhtar]

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

Re: Minor issues in .pgpass

[Hamid Akhtar]

On Wed, Mar 4, 2020 at 4:54 PM Fujii Masao 
wrote:

>
>
> On 2020/03/04 20:39, Hamid Akhtar wrote:
> >
> >
> > On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao  > wrote:
> >
> >
> >
> > On 2020/03/03 21:38, Hamid Akhtar wrote:
> >  >
> >  >
> >  > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <
> masao.fu...@oss.nttdata.com   masao.fu...@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)
> +   {
> +   chartmp[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
> 

Re: Minor issues in .pgpass

[Fujii Masao]

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



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



On 2020/03/03 21:38, Hamid Akhtar wrote:
 >
 >
 > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao mailto:masao.fu...@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)
+   {
+   chartmp[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

Re: Minor issues in .pgpass

[Hamid Akhtar]

On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao 
wrote:

>
>
> On 2020/03/03 21:38, Hamid Akhtar wrote:
> >
> >
> > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao  > 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.akh...@highgo.ca
SKYPE: engineeredvirus

Re: Minor issues in .pgpass

[Fujii Masao]

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

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



On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao mailto:masao.fu...@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
diff --git a/src/interfaces/libpq/fe-connect.c 
b/src/interfaces/libpq/fe-connect.c
index 408000af83..0157c619aa 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
charbuf[LINELEN];
@@ -7014,10 +7015,42 @@ passwordFromFile(const char *hostname, const char 
*port, const char *dbname,
   *p1,
   *p2;
int len;
+   int buflen;
 
if (fgets(buf, sizeof(buf), fp) == NULL)
break;
 
+   line_number++;
+   buflen = strlen(buf);
+   if (buflen >= sizeof(buf) - 1 && buf[buflen - 1] != '\n')
+   {
+   charrest[LINELEN];
+   int restlen;
+
+   /*
+* 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(rest, sizeof(rest), fp) == NULL)
+   break;
+   restlen = strlen(rest);
+   if (restlen < sizeof(rest) - 1 || rest[restlen 
- 1] == '\n')
+   break;
+   }
+   }
+
+   /* ignore comments */
+   if (buf[0] == '#')
+   continue;
+
/* strip trailing newline and carriage return */
len = pg_strip_crlf(buf);
 

Re: Minor issues in .pgpass

[Fujii Masao]

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



On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao mailto:masao.fu...@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

Re: Minor issues in .pgpass

[Hamid Akhtar]

On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar  wrote:

>
>
> On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao 
> 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)
>> > +   {
>> > +   chartmp[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.akh...@highgo.ca
> SKYPE: engineeredvirus
>


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

Re: Minor issues in .pgpass

[Hamid Akhtar]

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao 
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)
> > +   {
> > +   chartmp[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.akh...@highgo.ca
SKYPE: engineeredvirus

Re: Minor issues in .pgpass

[Fujii Masao]

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)
+   {
+   chartmp[LINELEN];


Regards,

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

Re: Minor issues in .pgpass

[Hamid Akhtar]

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
charbuf[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)
+   {
+   chartmp[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.akh...@highgo.ca
SKYPE: engineeredvirus

The new status of this patch is: Waiting on Author

Re: Minor issues in .pgpass

[Fujii Masao]

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
diff --git a/src/interfaces/libpq/fe-connect.c 
b/src/interfaces/libpq/fe-connect.c
index 408000af83..eb34d2122f 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
charbuf[LINELEN];
@@ -7014,10 +7015,42 @@ passwordFromFile(const char *hostname, const char 
*port, const char *dbname,
   *p1,
   *p2;
int len;
+   int buflen;
 
if (fgets(buf, sizeof(buf), fp) == NULL)
break;
 
+   line_number++;
+   buflen = strlen(buf);
+   if (buflen >= sizeof(buf) - 1 && buf[buflen - 1] != '\n')
+   {
+   chartmp[LINELEN];
+   int tmplen;
+
+   /*
+* 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;
+   tmplen = strlen(tmp);
+   if (tmplen < sizeof(tmp) -1 || tmp[tmplen - 1] 
== '\n')
+   break;
+   }
+   }
+
+   /* ignore comments */
+   if (buf[0] == '#')
+   continue;
+
/* strip trailing newline and carriage return */
len = pg_strip_crlf(buf);
 

Re: Minor issues in .pgpass

[David Fetter]

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  http://fetter.org/
Phone: +1 415 235 3778

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

Re: Minor issues in .pgpass

[Daniel Gustafsson]

> On 21 Jan 2020, at 07:27, Fujii Masao  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

Minor issues in .pgpass

[Fujii Masao]

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.

(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. Whether a line
   begins with # or not, libpq just checks that the first token
   in the line match with the host. That is, if you try to connect
   to the host with the hostname beginning with #,
   it can match to the line beginning with # in .pgpass.

   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.

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

I've not created the patch yet, but will do if we reach to
the consensus.

Regards,

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