On Wed, Mar 4, 2020 at 4:54 PM Fujii Masao <masao.fu...@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.fu...@oss.nttdata.com > <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 < > masao.fu...@oss.nttdata.com <mailto:masao.fu...@oss.nttdata.com> <mailto: > masao.fu...@oss.nttdata.com <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? > 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.akh...@highgo.ca SKYPE: engineeredvirus
