Re: Minor issues in .pgpass - Mailing list pgsql-hackers

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,
butit 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
proceedon 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
statementto "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