Re: Minor issues in .pgpass

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

>
>
> On 2020/03/04 20:39, Hamid Akhtar wrote:
> >
> >
> > On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com
> <mailto:masao(dot)fujii(at)oss(dot)nttdata(dot)com>> wrote:
> >
> >
> >
> > On 2020/03/03 21:38, Hamid Akhtar wrote:
> > >
> > >
> > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <
> masao(dot)fujii(at)oss(dot)nttdata(dot)com <mailto:masao(dot)fujii(at)oss(dot)nttdata(dot)com> <mailto:
> masao(dot)fujii(at)oss(dot)nttdata(dot)com <mailto:masao(dot)fujii(at)oss(dot)nttdata(dot)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(dot)akhtar(at)highgo(dot)ca
SKYPE: engineeredvirus