On 2020/03/04 20:39, Hamid Akhtar wrote:
On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <[email protected]
<mailto:[email protected]>> wrote:
On 2020/03/03 21:38, Hamid Akhtar wrote:
>
>
> On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <[email protected]
<mailto:[email protected]> <mailto:[email protected]
<mailto:[email protected]>>> 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
